Skip to content

Make get_model_builder public #6560

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

Merged
merged 2 commits into from
Sep 12, 2022
Merged

Conversation

datumbox
Copy link
Contributor

This PR makes it possible to get a model builder method by its name.

This is useful because a model might be registered under a different name than the method’s name (see quantization models, for example quantized_resnet50) and thus fetching it by indexing the module __dict__ might not be possible. This PR addresses feedback received internally by MobileVision and will simplify the integration with fastai (see fastai/fastai#3789).

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing find_model into get_model_builder was originally suggested in #6333 (comment) and in #6333 (comment) but it wasn't accepted at the time. What is the reason to change it now?

Also considering model buillders are now available, does it still make sense to expose get_model()? IIUC get_model(name, args) is now just get_model_builder(name)(args), so we would have 2 ways of doing the same thing?

@datumbox datumbox merged commit cac4e22 into pytorch:main Sep 12, 2022
@datumbox datumbox deleted the get_model_builder branch September 12, 2022 09:05
@datumbox
Copy link
Contributor Author

@NicolasHug Just saw your message.

What is the reason to change it now?

It was a private method named similarly to [find()](https://github.com/pytorch/vision/blob/main/torchvision/prototype/datasets/_api.py#L38) from datasets. Now that we publicly expose it, we can adopt your original proposal.

so we would have 2 ways of doing the same thing?

I don't mind particularly offering both and the user feedback so far has been quite positive for the new get_model() method.

@NicolasHug
Copy link
Member

NicolasHug commented Sep 12, 2022

I assume that the user feedback about get_model() came before get_model_builder() was exposed? It's still worth re-considering IMO.

There should be one-- and preferably only one --obvious way to do it.

@NicolasHug Just saw your message.

I'll try to accept that as true, but in all honesty, this is the exact same sequence of events as they happened in #6333. It makes me feel uncomfortable and I don't want to re-live that again. I'll disengage from this conversation.

facebook-github-bot pushed a commit that referenced this pull request Sep 12, 2022
Reviewed By: YosuaMichael

Differential Revision: D39426988

fbshipit-source-id: 87e4d5da57065e5c422ca3316f4cce32e515f978
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants