Remove plural types and aliases for native pytorch types #677

Landanjs · 2022-03-05T01:57:15Z

I may have misunderstood the assignment, but attempting to address #640 and #510.

Essentially:

Added a check for range objects in ensure_tuple() since these were the last type of Sequence we did not support.
Changed T | Tuple[T] | List[T] in Many to T | Sequence[T]
Changed docstrings and instances of T | Tuple | List to Many

If this actually looks right, I will add tests for ensure_tuple and every sequence type.

Side note: I'm a little confused when to use PytorchSchedulers vs ComposerSchedulers

ravi-mosaicml · 2022-03-05T02:26:51Z

There's some additional typing stuff will need to be adjust in iter_helpers.pyi -- specifically we'll need an additional overload methods to take a generic sequence and for str, bytes, and range objects.

composer/core/state.py

composer/core/types.py

composer/core/state.py

ravi-mosaicml · 2022-03-07T16:53:13Z

composer/utils/iter_helpers.py



-def ensure_tuple(x):
+def ensure_tuple(x) -> Tuple[Any, ...]:


iter_helpers.py is the exception to the rule and does not contain any type annotations itself. Instead, iter_helpers.pyi contains all the type annotations for iter_helpers.py

composer/utils/iter_helpers.py

ravi-mosaicml · 2022-03-07T16:55:03Z

composer/utils/iter_helpers.py

-    if isinstance(x, tuple):
-        return x
-    if isinstance(x, list):
+    if isinstance(x, (tuple, list, range)):


Suggested change

if isinstance(x, (tuple, list, range)):

if isinstance(x, (tuple, list, range, collections.abc.Sequence)):

ravi-mosaicml · 2022-03-07T16:57:12Z

Side note: I'm a little confused when to use PytorchSchedulers vs ComposerSchedulers

Good point; this will need to be clarified in the docs somewhere. At a high level, ComposerSchedulers are acceptable to use when constructing the trainer, but they are converted to PyTorchSchedulers which are used internally. (ComposerSchedulers simply return the learning rate factor and don't update the optimizer; instead, the trainer "compiles" them into a PyTorch LambdaLR scheduler)

…/composer into landan/fix_ensure_tuple

Landanjs · 2022-03-07T19:54:07Z

I think I addressed most comments, but I'm not very familiar with interfaces so it's likely a set that up wrong 😅.

I will start making tests now

I have a couple of docstring consistency questions that maybe should be addressed somewhere else:

Should I remove the :attr:from types in the state docstring (specifically inState`)? They don't seem necessary, but not sure if that is the appropriate format. The dataloaders don't seem to have these.
Should we use the aliases Tensor and Optimizer in docstrings? To me, it adds an extra click to figure out the actual type with little benefit.
Should Algorithms and Callbacks have the same type format as optimizers and schedulers i.e. they accept a single object or a sequence of objects? Right now, they are Sequence[Algorithms]
To align with removing plurals, should Tensors -> Tensor | Sequence[Tensor] in docstrings?

…/composer into landan/fix_ensure_tuple

Landanjs · 2022-03-07T21:14:29Z

Errr, there are a lot of Sequence object errors now... For example:

    first_optimizer = state.optimizers[0]
    expected_param_groups = deepcopy(first_optimizer.param_groups)

The second line gets error: Cannot access member "param_groups" for type "Sequence[Optimizer]". Do these need to be manually cast?

ravi-mosaicml · 2022-03-10T23:58:19Z

I think I addressed most comments, but I'm not very familiar with interfaces so it's likely a set that up wrong sweat_smile.

I will start making tests now

I have a couple of docstring consistency questions that maybe should be addressed somewhere else:

Should I remove the :attr:from types in the state docstring (specifically inState`)? They don't seem necessary, but not sure if that is the appropriate format. The dataloaders don't seem to have these.

It depends on what is being referenced; sometimes, :attr: is needed to get sphinx to link properly.
Ideally, we would use :attr: notation for every attribute, but I think we're doing it only for stuff not set on init.

Should we use the aliases Tensor and Optimizer in docstrings? To me, it adds an extra click to figure out the actual type with little benefit.

Good point; let's go with the torch types. I'd eventually like to remove types.py or make it significantly leaner.

Should Algorithms and Callbacks have the same type format as optimizers and schedulers i.e. they accept a single object or a sequence of objects? Right now, they are Sequence[Algorithms]

Yes, let's do the "singleton or sequence" approach.

To align with removing plurals, should Tensors -> Tensor | Sequence[Tensor] in docstrings?

Yup!

ravi-mosaicml · 2022-03-11T00:00:09Z

Errr, there are a lot of Sequence object errors now... For example:
    first_optimizer = state.optimizers[0]
    expected_param_groups = deepcopy(first_optimizer.param_groups)
    
The second line gets error: Cannot access member "param_groups" for type "Sequence[Optimizer]". Do these need to be manually cast?

It shouldn't need a manual cast; double check the type annotation for ensure_tuple.

ravi-mosaicml · 2022-03-11T01:15:00Z

@Landanjs I pushed a commit that (hopefully?) fixes the type errors with the ensure_tuple change.
If you could finish up the docs fixes in this PR, that'd be awesome.

Landanjs · 2022-03-14T17:01:21Z

The recent change tries to:

Use PyTorch's Tensor, Optimizer, Module classes where possible
Remove Tenors
Attempts to fix missing docstring links by prepending types.
Remove the illusion of multiple optimizer from docstrings

I did not convert Callbacks and Loggers to "singleton or sequence" format since these are converted into lists instead of tuples and I'm not confident the functionality is the same? But maybe should be changed in the future

ravi-mosaicml

Looking good! Main feedback is to remove the plural types from types.py (sorry I didn't mention this before) and the rest of the codebase (so we'll specify Unions in the type annotations, where necessary). Then I think it should be good to merge!

ravi-mosaicml · 2022-03-14T17:34:26Z

composer/core/types.py

        initialize optimizers.
    Tensor (torch.Tensor): Alias for :class:`torch.Tensor`.
-    Tensors (Tensor | Tuple[Tensor, ...] | List[Tensor]): Commonly used to represent e.g. a set of inputs,
+    Tensors (torch.Tensor | Sequence[torch.Tensor]): Commonly used to represent e.g. a set of inputs,


Can you remove this docstring (and the associated type alias below in the code)?

ravi-mosaicml · 2022-03-14T17:34:38Z

composer/core/types.py

    Metrics (Metric | MetricCollection): Union type covering common formats for representing metrics.
    Optimizer (torch.optim.Optimizer): Alias for :class:`torch.optim.Optimizer`
-    Optimizers (Optimizer | List[Optimizer] | Tuple[Optimizer, ...]): Union type for indeterminate amounts of optimizers.
+    Optimizers (torch.optim.Optimizer | Sequence[torch.optim.Optimizer]): Union type for indeterminate amounts of


Same as above -- can you remove this docstring (and the associated type alias below in the code)?

ravi-mosaicml · 2022-03-14T17:34:43Z

composer/core/types.py

        as :class:`torch.optim.lr_scheduler.ConstantLR`
    Scaler (torch.cuda.amp.grad_scaler.GradScaler): Alias for :class:`torch.cuda.amp.GradScaler`.
    JSON (str | float | int | None | List['JSON'] | Dict[str, 'JSON']): JSON Data
    Evaluators (Many[Evaluator]): Union type for indeterminate amounts of evaluators.


Same as above -- can you remove this docstring (and the associated type alias below in the code)?

ravi-mosaicml · 2022-03-14T17:35:02Z

composer/core/types.py


 T = TypeVar('T')
-Many = Union[T, Tuple[T, ...], List[T]]
+Many = Union[T, Sequence[T]]


Let's remove Many too

ravi-mosaicml · 2022-03-14T17:35:09Z

composer/core/types.py


 Tensor = torch.Tensor
-Tensors = Many[Tensor]
+Tensors = Union[Tensor, Sequence[Tensor]]


Same thing...removing the aliases here will ensure that we removed them throughout the entire codebase, and use the underlying Union[X, Sequence[X]] notation in the codebase throughout.

ravi-mosaicml · 2022-03-14T17:36:49Z

composer/trainer/trainer.py

            .. seealso:: :mod:`composer.optim` for the different optimizers built into Composer.
-        schedulers (Schedulers, optional): The learning rate schedulers. If ``[]`` or ``None``, will be set to
-            ``[constant_scheduler]``. (default: ``None``).
+        schedulers (types.PyTorchScheduler or Sequence[types.PyTorchScheduler], optional):


The trainer takes both pytorch schedulers or composer schedulers.

Suggested change

schedulers (types.PyTorchScheduler or Sequence[types.PyTorchScheduler], optional):

schedulers (types.ComposerScheduler | types.PyTorchScheduler | Sequence[types.ComposerScheduler | types.PyTorchScheduler], optional):

ravi-mosaicml · 2022-03-14T17:37:31Z

composer/trainer/trainer.py

-        schedulers (Schedulers, optional): The learning rate schedulers. If ``[]`` or ``None``, will be set to
-            ``[constant_scheduler]``. (default: ``None``).
+        schedulers (types.PyTorchScheduler or Sequence[types.PyTorchScheduler], optional):
+            The learning rate schedulers. If ``[]`` or ``None``, will be set to ``[constant_scheduler]``.


Suggested change

The learning rate schedulers. If ``[]`` or ``None``, will be set to ``[constant_scheduler]``.

The learning rate schedulers. If ``[]`` or ``None``, the learning rate will be constant.

…/composer into landan/fix_ensure_tuple

ravi-mosaicml · 2022-03-16T18:10:16Z

There appear to be a few docstring formatting warnings that need to be fixed (Docstring warnings are treated as errors and cause the build to fail).

00:04:06.043  /home/mosaicml/.local/lib/python3.9/site-packages/composer/trainer/trainer.py:docstring of composer.trainer.trainer.Trainer:39: WARNING: Field list ends without a blank line; unexpected unindent.
00:04:06.043  /home/mosaicml/.local/lib/python3.9/site-packages/composer/trainer/trainer.py:docstring of composer.trainer.trainer.Trainer:42: WARNING: Inline substitution_reference start-string without end-string.
00:04:06.043  /home/mosaicml/.local/lib/python3.9/site-packages/composer/trainer/trainer.py:docstring of composer.trainer.trainer.Trainer:44: WARNING: Definition list ends without a blank line; unexpected unindent.

Remove re-exported types from types.py. This should help with code readability, as now the underlying type is directly imported.

`Dict[str, Any]` is almost as short but clearer on what it is (a dictionary with string keys).

ravi-mosaicml

LGTM!

Landanjs added 3 commits March 4, 2022 17:39

First pass fix ensure_tuple

ba186dd

Remove endline

c773142

Adjust state docstring

8115fc5

Landanjs requested a review from ravi-mosaicml March 5, 2022 01:57

This was linked to issues Mar 5, 2022

Update ensure_tuple to support generic sequences for non-strings. #640

Closed

Confusing Scheduler and Schedulers types #510

Closed

Landanjs changed the title ~~First pass fix ensure_tuple~~ Fix ensure_tuple() to work with all Sequence types Mar 5, 2022

Landanjs added 2 commits March 4, 2022 17:58

Merge branch 'dev' into landan/fix_ensure_tuple

aaad3cd

Adjust docstring

403d480

ravi-mosaicml reviewed Mar 7, 2022

View reviewed changes

Merge branch 'dev' into landan/fix_ensure_tuple

ccedf7e

Landanjs added 2 commits March 7, 2022 11:49

Address comments

da1f28f

Merge branch 'landan/fix_ensure_tuple' of https://github.com/Landanjs…

313ecd5

…/composer into landan/fix_ensure_tuple

Merge branch 'dev' into landan/fix_ensure_tuple

68fddb6

Landanjs requested a review from ravi-mosaicml March 7, 2022 19:54

Landanjs added 5 commits March 7, 2022 12:01

Ooops typo

cbb9946

Merge branch 'landan/fix_ensure_tuple' of https://github.com/Landanjs…

df4f51f

…/composer into landan/fix_ensure_tuple

Add tests

953db55

Lint

869342b

Fix types

a8d5896

Merge branch 'dev' into landan/fix_ensure_tuple

5c35759

ravi-mosaicml mentioned this pull request Mar 8, 2022

Updated contributing.md and the style guide #670

Merged

Fixed type errors

c7e0dac

Landanjs added 2 commits March 14, 2022 09:56

Docstring styling

4dc3743

Merge branch 'dev' into landan/fix_ensure_tuple

a00d0f8

ravi-mosaicml reviewed Mar 14, 2022

View reviewed changes

Landanjs added 3 commits March 14, 2022 10:55

Errr docstring links?

f3bf6db

Merge branch 'landan/fix_ensure_tuple' of https://github.com/Landanjs…

801c6c2

…/composer into landan/fix_ensure_tuple

Merge branch 'dev' into landan/fix_ensure_tuple

60302ae

ravi-mosaicml mentioned this pull request Mar 14, 2022

Remove Batch Types - Part 1: recursive to_device function #727

Merged

ravi-mosaicml added this to the v0.4.2 milestone Mar 14, 2022

Landanjs added 4 commits March 14, 2022 13:52

Attempt at removing plural types pt. 1

8d9ae51

Merge branch 'landan/fix_ensure_tuple' of https://github.com/Landanjs…

85bb373

…/composer into landan/fix_ensure_tuple

Remove plural types pt. 2

a94b429

Merge with dev :(

64587c0

Landanjs requested a review from ravi-mosaicml March 16, 2022 00:09

Merge branch 'dev' into landan/fix_ensure_tuple

be2fb2c

ravi-mosaicml and others added 8 commits March 16, 2022 12:19

Clean up types.py

3168e07

Remove re-exported types from types.py. This should help with code readability, as now the underlying type is directly imported.

Rearranged import

06398c1

Fix circular import

452891e

Fix doctests

9e89c6b

Replaced StateDict type annotation with Dict[str, Any]

a6d6c9e

`Dict[str, Any]` is almost as short but clearer on what it is (a dictionary with string keys).

Removed some more plural types

25f8e93

Cleanup tensor_to_device

0b8faab

Fixed typo

763a204

ravi-mosaicml approved these changes Mar 16, 2022

View reviewed changes

ravi-mosaicml and others added 2 commits March 16, 2022 14:34

Allowing generic sequences

d2a3c43

Fix LoggerDestination issues

488c118

Landanjs changed the title ~~Fix ensure_tuple() to work with all Sequence types~~ Remove plural types and aliases for native pytorch types Mar 16, 2022

Landanjs merged commit 9387d5c into mosaicml:dev Mar 16, 2022

Landanjs deleted the landan/fix_ensure_tuple branch March 16, 2022 22:27

	if isinstance(x, (tuple, list, range)):
	if isinstance(x, (tuple, list, range, collections.abc.Sequence)):

	schedulers (types.PyTorchScheduler or Sequence[types.PyTorchScheduler], optional):
	schedulers (types.ComposerScheduler \| types.PyTorchScheduler \| Sequence[types.ComposerScheduler \| types.PyTorchScheduler], optional):

	The learning rate schedulers. If ``[]`` or ``None``, will be set to ``[constant_scheduler]``.
	The learning rate schedulers. If ``[]`` or ``None``, the learning rate will be constant.

Remove plural types and aliases for native pytorch types #677

Remove plural types and aliases for native pytorch types #677

Uh oh!

Conversation

Landanjs commented Mar 5, 2022

Uh oh!

ravi-mosaicml commented Mar 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravi-mosaicml commented Mar 7, 2022

Uh oh!

Landanjs commented Mar 7, 2022

Uh oh!

Landanjs commented Mar 7, 2022

Uh oh!

ravi-mosaicml commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ravi-mosaicml commented Mar 11, 2022

Uh oh!

ravi-mosaicml commented Mar 11, 2022

Uh oh!

Landanjs commented Mar 14, 2022

Uh oh!

ravi-mosaicml left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravi-mosaicml commented Mar 16, 2022

Uh oh!

ravi-mosaicml left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ravi-mosaicml commented Mar 10, 2022 •

edited

Loading