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

Standardize errors for when pre-trained weights are not available #5572

Closed
wants to merge 3 commits into from

Conversation

datumbox
Copy link
Contributor

@datumbox datumbox commented Mar 9, 2022

Our current implementation throws a ValueError if pre-trained weights are requested for an architecture that we don't offer checkpoints. In this PR we switch to NotImplementedError exceptions and we standardize the error messages.

@facebook-github-bot
Copy link

facebook-github-bot commented Mar 9, 2022

💊 CI failures summary and remediations

As of commit 394aa03 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

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.

Thanks for the PR @datumbox . What is the logic for using a NotImplementedError here? At first sight it seems that ValueError may be more relevant. From the docs of the NotImplementedError exception:

It should not be used to indicate that an operator or method is not meant to be supported at all

torchvision/models/convnext.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jdsgomes jdsgomes left a comment

Choose a reason for hiding this comment

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

I left a bit but overall I'm aligned with Nicolas point that NotImplemented might not be the best candidate here

torchvision/models/detection/faster_rcnn.py Show resolved Hide resolved
@datumbox
Copy link
Contributor Author

datumbox commented Mar 9, 2022

@NicolasHug I think you changed your opinion on this because this change was first proposed by you here: #5022 (comment)

Personally I think throwing ValueError is not the right option for this. The models can throw multiple ValueError exceptions if not parameterized correctly. Throwing NotImplementedError for something that is not supported is in the spirit of the docs.

@NicolasHug
Copy link
Member

@NicolasHug I think you changed your opinion on this because this change was first proposed by you here: #5022 (comment)

For the models modified in this PR, are we absolutely sure that we will implement the pretrained weights in the future (as it was the case in #5022 (comment)) ? If not, the Python docs explicitly recommend against using NotImplementedError.

@datumbox
Copy link
Contributor Author

datumbox commented Mar 9, 2022

@NicolasHug Yes, there is nothing stopping us from providing these models on the future and nor I can think of a case were we offer a model builder but we absolutely don't want to offer weights. Also note that the models missing are on the list to be added and can be contributed by the community #2707

@datumbox datumbox requested review from jdsgomes and NicolasHug March 9, 2022 15:34
@NicolasHug
Copy link
Member

I believe we rely on ValueError much more often than on NotImplementedError in the current code-base for such scenarios. I won't fight against it but sorry, I prefer not to approve it.

@datumbox datumbox closed this Mar 9, 2022
@datumbox datumbox deleted the hackathon/correct_exceptions branch March 9, 2022 19:07
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