Skip to content

Conversation

dskhudia
Copy link
Contributor

For evaluator.py, precision.py, serializable.py, state.py, time.py and types.py in composer.core

@dskhudia dskhudia force-pushed the daya/core_docs branch 4 times, most recently from 8316e0f to 80073b1 Compare February 25, 2022 16:24
Copy link
Contributor

@hanlint hanlint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one small nit on trying to reduce the verbosity of the comments by doing:
:class:.ComposerModel instead of :class:~.base.ComposerModel.

@ravi-mosaicml
Copy link
Contributor

ravi-mosaicml commented Mar 1, 2022

Overall looks good; here are some comments:

Copy link
Contributor

@ravi-mosaicml ravi-mosaicml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See narrative comments.

@dskhudia
Copy link
Contributor Author

dskhudia commented Mar 1, 2022

@ravi-mosaicml : Fixed most of it but need clarification on the following:

Also, I think it would be helpful to link to the timing guide

How do we usually link non-api references? using full path such as https://docs.mosaicml.com/en/v0.4.0/trainer/time.html?

can you move the serialized_attributes from being a class-level member to something defined on self.?

We are not mutating serialized_attributes

Some types are not autolinking

I forced link them. They are not getting picked by the Sphinx automatically.

our custom plural types in the docstring can be confusing. Can we enumerate this out?

Are you suggesting that we get rid of them ?

@hanlint
Copy link
Contributor

hanlint commented Mar 2, 2022

How do we usually link non-api references?

You can use :doc: directive, e.g. :doc:Time Guide</trainer/time>

Are you suggesting that we get rid of them ?

No, I think describe the plural options. e.g. Schedulers can be pytorch schedulers, our composer schedulers, or a list or tuple of these schedulers.

@ravi-mosaicml
Copy link
Contributor

ravi-mosaicml commented Mar 2, 2022

Also, I think it would be helpful to link to the timing guide

How do we usually link non-api references? using full path such as https://docs.mosaicml.com/en/v0.4.0/trainer/time.html?

It should be possible to use :doc: or :ref: notation.

can you move the serialized_attributes from being a class-level member to something defined on self.?

We are not mutating serialized_attributes

The deepspeed checkpointing logic does modify serialized_attributes so it can skip serialization of the models and optimizers on the state dict.

Some types are not autolinking

I forced link them. They are not getting picked by the Sphinx automatically.

KK, we can address in a separate PR.

our custom plural types in the docstring can be confusing. Can we enumerate this out?

Are you suggesting that we get rid of them ?

Actually, don't worry about this one. It will depend on #640. Generally speaking, we will want to remove the plural types from the docstrings and instead replace it with x | Sequence[x] It will probably be better to replace this all at once later.

@hanlint hanlint added the documentation Improvements or additions to documentation label Mar 2, 2022
@dskhudia dskhudia force-pushed the daya/core_docs branch 2 times, most recently from 606fb3b to fa61a7e Compare March 2, 2022 20:32
@dskhudia
Copy link
Contributor Author

dskhudia commented Mar 2, 2022

@ravi-mosaicml, @hanlint Addressed all comments.

@hanlint hanlint merged commit aa13f29 into dev Mar 2, 2022
@hanlint hanlint deleted the daya/core_docs branch March 2, 2022 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants