Make WeightEnum and Weights public + cleanups #7100
Conversation
|
@adamjstewart we're making the |
|
Yes, we would like Weights to be public too. Otherwise, other libraries can't add their own Weights. Another issue I noticed during our multi-weight API refactor was that the registration mechanism isn't public so all of the builtin |
NicolasHug
changed the title
|
Thanks for the feedback - @pmeier I think this is ready for review - LMK if you think we should add tests. I'd rather leave the docs for another PR one we get more feedback from @adamjstewart , but happy to hear your thoughts.
I'll comment on #6365 |
torchvision/models/__init__.py
Outdated
| # We're making Weights and WeightsEnum public for packages like torchgeo who are | ||
| # interested in using them https://github.com/pytorch/vision/issues/7094 |
There was a problem hiding this comment.
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.
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.
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.
I updated the comment, hopefully the intent is clear now!
There was a problem hiding this comment.
Yes, I like the wording better now.
torchvision/models/__init__.py
Outdated
|
|
||
| # 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.
Yes. Maybe even in this PR?
There was a problem hiding this comment.
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?
I typically think it shouldn't be part of the already quite dense models doc page.
There was a problem hiding this comment.
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.
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 torchvision.models).
There was a problem hiding this comment.
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.
NicolasHug
changed the title
|
Hey @NicolasHug! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
Reviewed By: vmoens Differential Revision: D43116103 fbshipit-source-id: 8283b5f1a94dd934f9f7c87e6ec5492733ed910a
Towards #7094