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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make WeightsEnum public #7094

Closed
adamjstewart opened this issue Jan 16, 2023 · 4 comments
Closed

Make WeightsEnum public #7094

adamjstewart opened this issue Jan 16, 2023 · 4 comments

Comments

@adamjstewart
Copy link
Contributor

馃殌 The feature

I propose making torchvision.models._api.WeightsEnum public and adding documentation for it.

Motivation, pitch

TorchGeo is adopting torchvision's new multi-weight support API: microsoft/torchgeo#917

All of our weights subclass WeightsEnum, but this requires us to import directly from torchvision.models._api. Since this module is prefixed by an underscore, I'm assuming this class is private and subject to change. Also, since this class isn't documented, we have to silence Sphinx warnings about the missing parent class docs.

Alternatives

We could copy-n-paste your WeightsEnum implementation but code duplication is bad and I don't want to deal with legal. We could keep importing from a private module but it may change in the future. Are there any other suggested alternatives for other libraries trying to copy the same design? Maybe this could be moved into TorchData?

Additional context

@nilsleh

@NicolasHug
Copy link
Member

NicolasHug commented Jan 17, 2023

Thanks for the feature request @adamjstewart . It's great to see torchgeo adopt the same API!

I think we're open to make the Weights and WeightsEnum classes public. They might require a bit of cleanup before we can make them public, so I started doing that in #7100. Please feel free to take a look and provide feedback!

@adamjstewart
Copy link
Contributor Author

On the topic of WeightsEnum, I noticed that there is no md5 attribute and no checksumming done when downloading weights like there is for datasets. Should there be? (I can open a separate issue for this if you want)

@NicolasHug
Copy link
Member

Sure @adamjstewart , that sounds reasonable. A new issue sounds like the best way forward.

@NicolasHug
Copy link
Member

I believe this was addressed in #7100 and thus this issue can be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants