-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Make WeightEnum and Weights public + cleanups #7100
Changes from 6 commits
5d73cee
86dfbbb
5adcc0a
aa4d02a
da5cf43
4bce609
6609898
15901c1
450f63a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,4 +15,8 @@ | |
from .swin_transformer import * | ||
from .maxvit import * | ||
from . import detection, optical_flow, quantization, segmentation, video | ||
from ._api import get_model, get_model_builder, get_model_weights, get_weight, list_models | ||
|
||
# We're making Weights and WeightsEnum public for packages like torchgeo who are | ||
# interested in using them https://github.com/pytorch/vision/issues/7094 | ||
# TODO: we could / should document them publicly as well? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Maybe even in this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following up on my comment above about the fact that these are the first developer-facing tools, do you have thoughts on where / how we should document them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you are right. With our new maintainer guide, downstream libraries are safe in the sense that we put them under our BC policy. My only concern is discoverability. If we leave it as is, people that want something like this maybe don't know that it is there? Or do we expect them to look into our implementation anyway? cc @adamjstewart There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's a first, I'm tempted to leave them undocumented for now and see how it goes. Developers who need those would check the code IMO (or not, rightfully assuming that they're public since they can be imported from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From a TorchGeo perspective, we're fine with having to read the source code to figure out how to use the weights. But our docs try to link to the base class docs, so if the base class isn't documented we have to add an ignore. So long term, it would be cool if it was documented, but not a hard requirement. |
||
from ._api import get_model, get_model_builder, get_model_weights, get_weight, list_models, Weights, WeightsEnum |
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.
Not sure if that is required as a code comment? I mean we don't explain for all the other things as well. Aren't we just taking them as part of the public API now?
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.
They're sort of special in the sense that they're more developer tools intended to be used by downstream libraries, rather than your typical transform / model which are made for end users.
I don't think we have ever made any developer tool public yet, and I added the comment to indicate that this is not done by mistake. I won't fight to keep it though :)
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.
Good point, I didn't consider that. Maybe change the wording to something like "These are public ..."? My initial understanding was that this was meant as a comment for the reviewer or the PR not someone arbitrary looking at the code.
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.
I updated the comment, hopefully the intent is clear now!
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.
Yes, I like the wording better now.