Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to update type definition dynamically #44

Closed
jianglongye opened this issue Apr 15, 2023 · 3 comments
Closed

How to update type definition dynamically #44

jianglongye opened this issue Apr 15, 2023 · 3 comments

Comments

@jianglongye
Copy link

jianglongye commented Apr 15, 2023

Thanks for your fantastic project!

I am currently using NeRFStudio as an external dependency in my own project. However, I am facing an issue with the AnnotatedDataParserUnion type definition (located at here), which contains some pre-defined data parsers. My goal is to add a custom dataparser by updating this type definition dynamically. While it is easy to achieve this by modifying the NeRFStudio codebase directly, it is difficult to do so when using NeRFStudio as an external dependency.

I have noticed that some other vision codebases utilize a registry class that allows the registration of new classes on the fly. I was wondering if it would be possible for tyro to support a similar feature in the future. Additionally, I would like to suggest exploring the possibility of using more standard data structures such as list, dict, or class for subcommands config rather than a Union type. I am curious to hear your thoughts on this matter.

Thanks again!

@brentyi
Copy link
Owner

brentyi commented Apr 15, 2023

Hi @jianglongye!

Can you see if nerfstudio-project/nerfstudio#1622 helps at all? If you don't want to deal with the types, one option is that you can add a custom method config that just has the dataparser you want in it. I think @jake-austin had this working.

Aside from what's proposed in that PR nerfstudio does support a model registry (https://docs.nerf.studio/en/latest/developer_guides/new_methods.html), but I'm not super familiar with all the details of this.

Additionally, I would like to suggest exploring the possibility of using more standard data structures such as list, dict, or class for subcommands config rather than a Union type. I am curious to hear your thoughts on this matter.

With regards to this I can put some thought into it; this was the loose idea of subcommand_type_from_defaults but I'm sure there's room for improvement. The challenge is always to balance runtime flexibility with type safety.

@jianglongye
Copy link
Author

Thanks for pointing out the pull request and the new feature for model registry!

But my issue comes exactly from this model registry, which shows that we can dynamically add a custom method, but we can't dynamically add a custom data parser. The reason is that our method_configs are stored as a dict, and will search for custom methods before converting to a Union type.

I can see why you made this design choice. If we do a fully dynamic Registry, the specific type for this Registry will be unknown, since you can always add some entries to it during runtime (although I feel like this is okay).

@brentyi
Copy link
Owner

brentyi commented Apr 17, 2023

Okay, that makes sense. Perhaps one option is to add another registry for dataparsers in nerfstudio; if this is something you're interested feel free to file an issue there and we can see how the team feels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants