Add registration mechanism for models #6333

datumbox · 2022-07-29T14:27:38Z

Implements the proposal from RFC #6330

test/test_extended_models.py

NicolasHug · 2022-08-01T07:35:12Z

torchvision/models/_api.py

+    return sorted(models)
+
+
+def find_model(name: str) -> Callable[..., M]:


Reading the PR above, it's not obvious what the difference is between find_model and get_model. Maybe rename this to get_model_fn or get_model_builder?

Also might be worth prefixing with _, just to make clear that this is purely internal when reading the rest of the code.

The reasons it's named like this is to align with the naming on Datasets side. See here:

vision/torchvision/prototype/datasets/_api.py

Lines 38 to 41 in 6ca9c76

def find(dct: Dict[str, T], name: str) -> T:

name = name.lower()

try:

return dct[name]

But at any case the method is already internal as it's not exposed via the __all__ and it's in a private module.

NicolasHug · 2022-08-01T07:38:53Z

torchvision/models/_api.py

@@ -75,7 +78,9 @@ def __getattr__(self, name):

 def get_weight(name: str) -> WeightsEnum:
    """
-    Gets the weight enum value by its full name. Example: "ResNet50_Weights.IMAGENET1K_V1"
+    Gets the weights enum value by its full name. Example: "ResNet50_Weights.IMAGENET1K_V1"


Do we really want this and the rest to be Beta? I feel like we should reserve that for exceptional cases only. What is the plan to graduate these to stable?

I would rather release this as beta and depending on community feedback graduate before release than releasing to stable and then having to communicate it as beta on the last moment.

Do we have a way to remember to graduate this before the release?

I'm a bit concerned that we're going to start merging everything as Beta as soon as we're not 100% sure of it. Maybe it's a sign we need to think more about the PR? Eventually, this might lead to the Beta status having no meaningful definition.

I'm in discussions with the marketing team to see if we can post a blog post to inform people, similar to what I did with the multi weights API. Regardless, I believe that when a brand new mechanism is proposed, it has to go through the process of feedback and thus should be released as Beta. When I feel I got enough feedback about the new mechanism and especially the naming of the methods, I'll make sure to promote it to stable.

IMO we should use beta for things where we're clearly not sure, or where we foresee potentially mixed community feedback. Is this the case here?

Also, we should consider that in the future, the guideline might be that Beta APIs must come with a warning (which can potentially be disabled): pytorch/pytorch#72317, #6173, https://github.com/pytorch/pytorch/compare/master...albanD:feature_warning_system?expand=1.
I feel like this would be overkill here?

IMO we should use beta for things where we're clearly not sure, or where we foresee potentially mixed community feedback. Is this the case here?

Yes that's totally the case. As you can see from #6330, our team had different opinions on the naming of the methods. We should seek the input of the community.

I feel like this would be overkill here?

Yes I think adding warnings is too extreme and it's something that needs to be discussed separately.

If we're only concerned about the naming, then I don't think breaking user code for changing an imperfect name to another one is something reasonable to do.

Other than the naming, there are a few more API considerations that might arise such as the way the module parameter of list_models() works and Aligning the two registration APIs across Datasets and Models. More issues might pop up after we communicate the mechanism (I'm conscious that we all have blindspots). Overall I would much rather get input from the community on this than later introduce breaking changes in order to act upon their feedback. I think what we can do is try to push this mechanism via social and blogposts and collect feedback prior the release. If all is good, we should be able to promote to stable pretty fast aka before the next release.

torchvision/models/_api.py

jdsgomes · 2022-08-01T09:01:13Z

torchvision/models/_api.py

+    return fn
+
+
+def get_model(name: str, **config: Any) -> M:


+1 to Nicolas naming suggestion. My preference would be build_model and get_model_builder. This can be done in a separate PR and align to datasets or together with any changes that might arise from community feedback.
Shouldn't have missed it in the RFC.

Sounds good. Let's release is as beta to get input and we can promote to stable once we get enough positive feedback from the community. Concerning the details of this comment, the get_model() returns an initialized model it doesn't return the model builder. It's the find_model() that returns the model builder, but that's an internal method on the _api module and not exposed publicly. At any case, I agree it's worth reviewing the names on a follow up and ensure they are aligned with the mechanism proposed on the datasets.

I meant get_model -> build_model and find_model -> get_model_builder.

NicolasHug · 2022-08-01T10:32:35Z

there are a few more API considerations that might arise such as the way the module parameter of list_models() works and Aligning the two registration APIs across Datasets and Models

As discussed internally:

any change we make to the module parameter can be BC
aligning with the dataset API isn't constraining in any way since it's still prototype.

Merging this as Beta without clear potentially BC-breaking changes sets a precedent, and leaves the door open for marking any future APIs as Beta just for the sake of circumventing our deprecation policy. I'm sorry but I cannot support this change.

EDIT: clarification: I support the registration mechanism and the public-facing APIs that this PR introduces. I think they should however be stable (not Beta).

Summary: * Model registration mechanism. * Add overwrite options to the dataset prototype registration mechanism. * Adding example models. * Fix module filtering * Fix linter * Fix docs * Make name optional if same as model builder * Apply updates from code-review. * fix minor bug * Adding getter for model weight enum * Support both strings and callables on get_model_weight. * linter fixes * Fixing mypy. * Renaming `get_model_weight` to `get_model_weights` * Registering all classification models. * Registering all video models. * Registering all detection models. * Registering all optical flow models. * Fixing mypy. * Registering all segmentation models. * Registering all quantization models. * Fixing linter * Registering all prototype depth perception models. * Adding tests and updating existing tests. * Fix linters * Fix tests. * Add beta annotation on docs. * Fix tests. * Apply changes from code-review. * Adding documentation. * Fix docs. Reviewed By: NicolasHug Differential Revision: D38351757 fbshipit-source-id: adff0aa1a21e57e79fdbdbdb0a767121819da673

datumbox added 15 commits July 28, 2022 18:40

Model registration mechanism.

0d62aaf

Add overwrite options to the dataset prototype registration mechanism.

0e7eb8a

Adding example models.

1520566

Fix module filtering

2e16077

Fix linter

a02c124

Fix docs

eedf8df

Make name optional if same as model builder

a91a5b4

Apply updates from code-review.

abbe23e

fix minor bug

1eb8159

Adding getter for model weight enum

924388e

Support both strings and callables on get_model_weight.

bd2327a

linter fixes

a815a63

Fixing mypy.

9e4e62c

Renaming get_model_weight to get_model_weights

0209327

Registering all classification models.

2a63dce

datumbox added module: models new feature labels Jul 29, 2022

facebook-github-bot added the cla signed label Jul 29, 2022

datumbox and others added 11 commits July 29, 2022 15:34

Registering all video models.

976a93e

Registering all detection models.

2b8dc89

Registering all optical flow models.

040ddfc

Fixing mypy.

2031bf7

Registering all segmentation models.

1f27788

Registering all quantization models.

4d98f6c

Fixing linter

ba0bb82

Merge branch 'main' into models/registration_mechanism

648eb6e

Registering all prototype depth perception models.

0e2d120

Adding tests and updating existing tests.

2499c75

Fix linters

a75cfac

datumbox changed the title ~~[WIP] Add registration mechanism for models~~ Add registration mechanism for models Jul 29, 2022

datumbox requested a review from NicolasHug July 29, 2022 16:59

datumbox added 3 commits July 30, 2022 09:57

Fix tests.

7e9c2e7

Add beta annotation on docs.

81a7e3f

Fix tests.

867f85f

NicolasHug reviewed Aug 1, 2022

View reviewed changes

datumbox added 3 commits August 1, 2022 08:57

Apply changes from code-review.

7ca12b9

Adding documentation.

7c3a0ba

Fix docs.

2eae177

jdsgomes approved these changes Aug 1, 2022

View reviewed changes

Merge branch 'main' into models/registration_mechanism

efe1bd9

datumbox merged commit 0a919db into pytorch:main Aug 1, 2022

datumbox deleted the models/registration_mechanism branch August 1, 2022 10:09

NicolasHug mentioned this pull request Aug 4, 2022

Expose on Hub the public methods of the registration API #6364

Merged

NicolasHug mentioned this pull request Sep 12, 2022

Make get_model_builder public #6560

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add registration mechanism for models #6333

Add registration mechanism for models #6333

datumbox commented Jul 29, 2022

NicolasHug Aug 1, 2022

datumbox Aug 1, 2022

NicolasHug Aug 1, 2022

datumbox Aug 1, 2022

NicolasHug Aug 1, 2022

datumbox Aug 1, 2022 •

edited

Loading

NicolasHug Aug 1, 2022

datumbox Aug 1, 2022

NicolasHug Aug 1, 2022

datumbox Aug 1, 2022

jdsgomes Aug 1, 2022

datumbox Aug 1, 2022

jdsgomes Aug 1, 2022

NicolasHug commented Aug 1, 2022 •

edited

Loading

		return sorted(models)


		def find_model(name: str) -> Callable[..., M]:

	def find(dct: Dict[str, T], name: str) -> T:
	name = name.lower()
	try:
	return dct[name]

Add registration mechanism for models #6333

Add registration mechanism for models #6333

Conversation

datumbox commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datumbox Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasHug commented Aug 1, 2022 • edited Loading

datumbox Aug 1, 2022 •

edited

Loading

NicolasHug commented Aug 1, 2022 •

edited

Loading