Add filter parameters to list_models()

[MateuszGuzek]

Addressing Issue / comment: #6365 (comment)

We want to add a filtering mechanism to retrieve only a subset of models.

Includes include and exclude models functionalities.

[x] Documentation updated
[x] Test created

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7718

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 4 New Failures, 1 Unrelated Failure

As of commit 7ca68d9:

NEW FAILURES - The following jobs have failed:

• Build M1 Conda (gh)
• Build M1 Wheels (gh)
• unittests-linux (3.8, linux.g5.4xlarge.nvidia.gpu, cuda, 11.8) / linux-job (gh)
• unittests-windows (3.9, windows.4xlarge, cpu) / windows-job (gh)

BROKEN TRUNK - The following job failed but were present on the merge base 2d4484f:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• unittests-windows (3.10, windows.4xlarge, cpu) / windows-job (gh)

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[NicolasHug]

Thanks a lot for the PR @MateuszGuzek . I made some comments and suggestions below. The implementation looks great! For the tests I have some reservations at this point: the main one being that the tests seem to be a lot more complex than the code they're testing. I made some suggestions which hopefully will simplify a few things, we'll see how it goes. Definitely LMK what you think.

Thanks!

[NicolasHug]

I'd suggest

Suggested change

	if include_filters is not None:
	if include_filters:

So that cases like list_models(include_filters=[]) are equivalent to list_models(). (same below).

Most people won't be writing list_models(include_filters=[]) directly but it's possible to write list_models(include_filters=my_filter) where my_filter ends up being an empty list, or an empty string

[MateuszGuzek]

With this one I am not entirely sure, it is an edge case. Default is None, and it means include all. But if not None is passed, it is an explicit Iterable of what should be included.

The more strict the pattern, the less models you should return, with extreme cases of exact match, which result in a single model, and then one step further is to have an empty list, which for the sake of consistency could return nothing.

I imagine use case where you produce a list of include filters using set operations, and some of them may produce empty lists. Then I think it may be safer to fail fast (by ending up with no models), instead of suddenly flipping to accepting all possible models.

Please advise :)

[NicolasHug]

Thanks @MateuszGuzek , that makes sense. I guess there are cases to be made for both options.

@pmeier @vfdev-5 any thought on that? Basically we're trying to decide whether list_models(include=[]) should be interpreted as A) "there's no filter, so don't filter anything, return the whole list" or B) as "there's no filter, so there's nothing to include, so we return nothing".

Note that timm uses A (which IMHO is a good reason to go for that):

https://github.com/huggingface/pytorch-image-models/blob/c241081251323dfc5e8dc799d49740c48cc9096f/timm/models/_registry.py#L205

[pmeier]

Since the feature request in #6365 (comment) specifically mentions

timm has this functionality and it really boosts productivity.

we should align with what they have, i.e. go for A

[MateuszGuzek]

we should align with what they have, i.e. go for A

This is a fair point, I will go for this implementation.

[NicolasHug]

Thanks @MateuszGuzek, made another pass!

[MateuszGuzek]

Thanks for the comments, I applied all of them, it looks better now. :)

[NicolasHug]

Thanks a lot @MateuszGuzek , LGTM! I just pushed some minor cosmetics changes in the last commit (it's easier to address directly than to make another round of comments).

We still have a mypy error:

(pt) ➜  vision git:(filter_list_models) ✗ mypy torchvision/models/_api.py
torchvision/models/_api.py:231: error: Need type annotation for "models" (hint: "models: Set[<type>] = ...") 
[var-annotated]
            models = set()
            ^~~~~~
torchvision/models/_api.py:237: error: Incompatible types in assignment (expression has type "List[str]", variable has
type "Set[Union[Any, str]]")  [assignment]
            models = all_models
                     ^~~~~~~~~~

It is apparently too difficult for mypy to understand that sorted(some_set) returns a list. I'd suggest to silence mypy on this entire file unless @pmeier has a better solution.

[NicolasHug]

Thanks for the mypy fix @pmeier I'll merge when green