Simplify external projects, unify plugin API #2146
Conversation
| the same as the vanilla union, but results in shorter subcommand names.""" | ||
|
|
||
|
|
||
| def fix_method_dataparser_type(method_config: TrainerConfig): |
There was a problem hiding this comment.
@brentyi , can you please take a closer look at this function?
There was a problem hiding this comment.
This makes sense but feels scary to me, is there no way to keep the parsers annotated as AnnotatedDataParserUnion? Is the issue with circular imports? Would breaking this file up into smaller files (like one where the dataparser union is defined and one where the method union is defined) help?
There was a problem hiding this comment.
I think having the AnnotatedDataParserUnion in the DataManagerConfig is a bad design which leads to circular imports when you want to define the DataParser and something else (trainerconfig) in the same file. I agree that redefining the types here is not the safest. I think it would be preferable if there was a way to instruct tyro to do it instead: replace base types with the annotated unions? What do you think? If you like this solution I can try to implement it in tyro.
There was a problem hiding this comment.
Sorry for missing your reply, I had accidentally left my comment twice and assumed you hadn't replied yet because I was looking at the other one...
I think it would be preferable if there was a way to instruct tyro to do it instead: replace base types with the annotated unions
Yeah, this makes sense. There's an unfinished PR that would enable this here: brentyi/tyro#30
If you're willing to make a PR to tyro making this kind of thing possible — either building on my PR or starting from scratch — that'd of course be awesome, but I don't have a good sense of how long something like that would take (or how long getting ramped up on the tyro codebase would take).
If it's too much effort just splitting things like the DataParser + trainerconfig that you described into separate files seems like an unideal but workable short-term solution.
jkulhanek
force-pushed
the
jkulhanek/unify-plugin-registry
branch
3 times, most recently
from
20a30cb to
97567d7
Compare
jkulhanek
force-pushed
the
jkulhanek/unify-plugin-registry
branch
from
97567d7 to
68adfb6
Compare
| from nerfstudio.plugins.registry import discover_dataparsers, discover_methods | ||
|
|
||
|
|
||
| def merge_methods(methods, method_descriptions, new_methods, new_descriptions, overwrite=True): |
There was a problem hiding this comment.
while we're making changes, can we add annotations here?
| the same as the vanilla union, but results in shorter subcommand names.""" | ||
|
|
||
|
|
||
| def fix_method_dataparser_type(method_config: TrainerConfig): |
There was a problem hiding this comment.
This makes sense but feels scary to me, is there no way to keep the parsers annotated as AnnotatedDataParserUnion? Is the issue with circular imports? Would breaking this file up into smaller files (like one where the dataparser union is defined and one where the method union is defined) help?
This PR implements the following: