My feedback and thoughts

[mthrok]

Overall, it looks good. Introducing the dedicated class and adding preprocessing transform feels a major improvement.
The following is my thoughts reading through the README and examples.

Major points

• Do we want to have parent the Weights class? I think the proposition can also be an idiom of best practice for the flexibility, which different project can implement without having a parent class.
  • check_type method is nice but it is simple enough (isinstance) to perform the check. Also what do you think of allowing the customize the behavior with duck-typing, say, in-house models?
  • The treatment of latest: bool feels a bit tedious for maintenance. When I add a latest model, I would like to be care-free about the previously-latest model. Since only one of them is supposed to be `True, how about simply making it a hard-coded class attribute?
• The idea of latest: bool might not synthesize well with audio. Say, I have a model architecture for Speech Recognition, I can have multiple of SOTA/latest models because of language / expected environments. (Note it is common to train and deploy multiple models with the same architecture, so that one is optimized for meeting room dictation, another is optimized for phone-conversation etc...)
• What if a model needs post-processing? Is the current framework extendable? For example, when the preprocessing is FFT, then post-processing can be the inverse of the specific FFT applied during the preprocessing.

Minor points

• The naming Weights feels slightly off because transforms not only contains weights but also defines the preprocessing operations as well.
• state_dict method should accept **kwargs, which will be passed to load_state_dict_from_url, so that the downloading process can be customized. (i.e. download location and such)
• I found it so hard to make Sphinx work well with Enum. Did you try?
• Currently, partial is used for the definition of transforms. I think this is good, because it will not instantiate an object. But every maintainer needs to be careful not to accidentally instantiate a transform here, and maintainers have to remember it at code review.

[datumbox]

@mthrok Thanks a lot for the prompt feedback, very good points. I'll try to respond to as many as I can:

Major points

Do we want to have parent the Weights class?

Each Domain library should have its own implementation of base Weights class equivalent. It's not something that we will share across domains. Is this point clear from the RFC or you think it requires clarifications?

Concerning the use of a parent class within a domain project, I think it can be beneficial because:

• Such a base class can help you avoid defining over and over the same keys for the various models.
• Can help you group together some utility methods. The check_type and the state_dict are two simple examples but I think there can be more. Note that handling the model loading centrally has benefits if you want to overwrite it (manifold on FBcode).
• It will allow you to link model builder methods to the weights parameter easier by checking the types. This is something I indicate on the optional registration mechanism.

The idea of latest: bool might not synthesize well with audio.

Feel free to completely ignore the specific variable. It was meant to be there for illustration purposes of things you could define in the Weights class. If you have no use for it then your base class won't declare it.

What if a model needs post-processing?

Excellent point; Vision doesn't have post-processing. I think this means that your Weights class will also contains two transform fields. One for preprocessing and one for postprocessing. Because you have the freedom to implement the Weight class however it makes sense to your project, it means that you literally add anything that makes sense in there.

Minor points

The naming Weights feels slightly off

Naming is NP-hard. :) I'm open to other naming proposals.

state_dict method should accept **kwargs

Excellent point. My implementation was meant to be a simple one just to illustrate options but if you want send a PR and we can merge it in!

I found it so hard to make Sphinx work well with Enum. Did you try?

We are using Enums in multiple places on Vision without problems [1, 2, 3]

Currently, partial is used for the definition of transforms. I think this is good, because it will not instantiate an object. But every maintainer needs to be careful not to accidentally instantiate a transform here, and maintainers have to remember it at code review.

Yes, that's true. In domains where the transforms have no memory (like vision typically), it's not a big deal to instantiate the object immediately. But in domains like Text (and Audio?) Lazy loading is critical. This is why I adopted this strategy. I think we can setup a unit-test that fetches Weights subclasses automatically and ensures that the transform is a callable. Thoughts?

[datumbox]

@mthrok Many of your points raised in this ticket now are being discussed on separate issues.

Could you indicate which other of your points are not covered by my response so that we can open separate dedicated issues and then close this one? It might be easier to follow this way.

[mthrok]

So after giving some thought, I think renaming weights to something like bundle would communicate the concept better. With that we can close the issue.

[datumbox]

Thanks @mthrok. I agree that using good names is crucial. Could you please clarify if you propose to rename the Weights class or the WeightEntry to Bundle?

TBH I'm not sure that Bundle communicates accurately what this class encodes. It literally stores meta information about the weights, hence the current name. But I would like to think what others think as well on this before renaming. Thoughts @fmassa @parmeet ?

[mthrok]

It's WeightEntry class.

[datumbox]

Thanks for the clarification!

Cool, my reservation above was about renaming Weights. Since you meant WeightEntry, I don't have strong opinion. I can't really say I like the current name either, so I'm open to other proposals as well. I'll wait for others to comment before changing.