Models docstrings

[A-Jacobson]

#401

• not sure how far i should go with the loss.py, initializers.py and model architecture files. The first two probably need a good refactor, loss -> metrics and initializers -> (not sure, maybe deletion). The model architectures are copied from all over the place. Am adding docstrings but am reluctant to refactor. Our ComposerModel versions should be the public facing interface for these.

• unsure about module level docstring, should i do something like this? https://docs.mosaicml.com/en/v0.3.1/models.html. or link to the incoming model cards that @ajaysaini725 is writing to cover the model descriptions.

[ravi-mosaicml]

• not sure how far i should go with the loss.py, initializers.py and model architecture files. The first two probably need a good refactor, loss -> metrics and initializers -> (not sure, maybe deletion). The model architectures are copied from all over the place. Am adding docstrings but am reluctant to refactor. Our ComposerModel versions should be the public facing interface for these.

If a refactor is desperately needed, then let's do it. For PR review, though, could this refactor be done separately?

• unsure about module level docstring, should i do something like this? https://docs.mosaicml.com/en/v0.3.1/models.html. or link to the incoming model cards that @ajaysaini725 is writing to cover the model descriptions.

A stub that would link to the model cards works.

[hanlint]

Thanks for putting this together! A few comments:

ComposerModel:

• Can you add more information to the ComposerModel base class docstrings? e.g. especially the coupling of forward/loss and validate/metrics.

• forward: its worth mentiong that the input is from the dataloader. we want to tell folks as much about our trainer loop logic as possible. same thing for loss -- say that outputs come from forward, and batch from the dataloader

• metrics: similarly explain that the metrics shuld be torchmetrics. The "warning" should hide the torchmetrics link (e.g. See torchmetrics[link] for more details). Mention that one can return multiple metrics that would all be evaluated on the eval_dataloader.

• ComposerTransformer docstring should also be expanded with more information about how it works and example usages

• same for TIMM, BERT, ResNet, GPTmodel, etc, we need some code blocks that show its usage, and more detail on how it works.

• TIMM has some formatting issues with testcode and also the model_name and links in the parameters. See: https://mosaicml-composer--469.com.readthedocs.build/en/469/api_reference/composer.models.timm.model.html#composer.models.timm.model.Timm

[A-Jacobson]

Done:

• Added all file level docstrings.
• embedding links.
• added all the `` marks...everywhere.
• vit docstrings, sorta minimal.
• bare minimum ssd docstrings.
• refactored effecientnet to hide non user-facing blocks.
• removed initializers from resnet9 init as they weren't being used. you can add them back if you add the logic to actually apply them =)

Not Done:

• embedded paper links, they are cited as [Name](Link) not [Name](link)(Author et al, Date). Should i do (Author et al, Date)? Is there a relatively painless way to get this information?
• some links are still broken: transformers, base types, some torchmetrics. These broken links don't have readthedocs paths available for us. I've linked to the relevant documentation or github pages for some. I've left others alone either because we're working to remove them (Batch types), or they're in an awkward place to add a normal link (Return or Args). Open to suggestions here but I do think it's ok to send this off without them.
• codeblocks -> doctests in abstract classes, punting to a later PR as they require fixtures + some are psuedocode.
• State formatting on metrics, adding them as attrs will cause more trouble than living with the less pretty formatting for now.
• SSD is a lot.. I know it's not perfect but I'd like to get this PR moving before even more models appear.
• general refactoring (standardizing module naming and layout) is being punted to here, composer.models cleanup #536. This PR is already too crazy, lets end the pain.

@growlix I choose you!

[growlix]

Looking great, Austin. Hope your eyes/fingers aren't too bloody.

embedded paper links, they are cited as Name not Name(Author et al, Date). Should i do (Author et al, Date)? Is there a relatively painless way to get this information?

I'd go Name(Author et al, Date) if possible; Author et al, Date is also acceptable. I think author attribution is the most important part. Unfortunately I don't know if there's an easy way to do this other than clicking through to the actual page.

some links are still broken: transformers, base types, some torchmetrics. These broken links don't have readthedocs paths available for us. I've linked to the relevant documentation or github pages for some. I've left others alone either because we're working to remove them (Batch types), or they're in an awkward place to add a normal link (Return or Args). Open to suggestions here but I do think it's ok to send this off without them.

Agree. I think you can leave as-is.

codeblocks -> doctests in abstract classes, punting to a later PR as they require fixtures + some are psuedocode.

Sounds good, just remember to file an issue.

SSD is a lot.. I know it's not perfect but I'd like to get this PR moving before even more models appear.

I'll double check for anything egregious, but otherwise sounds good, file an issue.

State formatting on metrics, adding them as attrs will cause more trouble than living with the less pretty formatting for now.

As per Bandish's suggestion, I think you can just move the state info to a new paragraph in the summary and preface it with something like

Adds the following attributes to :class:`composer.core.state`:

(Don't quote my syntax on that)

Unfortunately I think the current rendering is a bit too sloppy.

general refactoring (standardizing module naming and layout) is being punted to here, #536. This PR is already too crazy, lets end the pain.

• 1

[A-Jacobson]

• author attribution in.
• state information moved above params in nlp_metrics not sure this is prettier but it's done.
• issue filed for codeblocks -> testcode base composermodel codeblocks -> testcode #703
• issue filed for ssd cleanup cleanup SSD docstrings #704

[growlix]

SSD files need __all__, but otherwise looks p good