-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Clean up purely informational fields from Weight Meta-data #5852
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
datumbox
commented
Apr 21, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some clarifications:
NicolasHug
approved these changes
Apr 21, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @datumbox , minor Qs but LGTM anyway
facebook-github-bot
pushed a commit
that referenced
this pull request
May 5, 2022
…5852) Summary: * Removing `task`, `architecture` and `quantization` * Fix mypy * Remove size field * Remove unused import. * Fix mypy * Remove size from schema list. * update todo * Simplify with assert * Adding min_size to all models. * Update RAFT min size to 128 Reviewed By: jdsgomes, NicolasHug Differential Revision: D36095646 fbshipit-source-id: 80caefd0dd39a250fa61687d6a8e77f709916ddd
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Continuation of work from #5848 and #5842 to clean up the meta-data of the Multi-weight support API.
This PR:
task
andarchitecture
from the meta-data as these can be inferred from the Weights object.size
which is already encoded in the transforms (similar to what we did forinterpolation
). The specific field has caused confusion over its meaning (see Port Multi-weight support from prototype to main TorchVision #5618 (comment) and Port Multi-weight support from prototype to main TorchVision #5618 (comment)).min_size
to all models and makes it a mandatory field.quantization
informational field from the Quantized models. This is likely that we will need to restore a field like this on the near future (depending on the work on [Quant] Add FX support in quantization examples #5797) to differentiate between models quantized with Eager vs FX graph mode API etc, but let's postpone this until we know all the details.With the above changes, the meta-data should contain fields that are intended to be manipulated programmatically hence we can guarantee BC. The only exception is the recipe URL which is there for informational purposes and can be changed to a different value on the future but this is need to to build an effective documentation. All purely informational fields, should be moved on the doc strings.