Skip to content

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Nov 8, 2022

Related to #6925 (comment)

dicts are ordered from Python 3.6 3.7, so we might not need to use typing.OrderedDict which is causing failures with 3.7.0 and which is deprecated in 3.9.

@@ -426,7 +426,7 @@ def __init__(
) -> None:
super().__init__()

layers: OrderedDict[str, Any] = OrderedDict() # type: ignore
layers: Dict[str, Any] = {}
Copy link
Contributor

@datumbox datumbox Nov 8, 2022

Choose a reason for hiding this comment

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

This dictionary was intended to maintain the order of layers so that the sequential call at like 469 will execute them in the right order.

Edit: think the order might be maintained for low number of keys but this is a not-guaranteed behaviour and might be different from platform to platform. I recommend restoring the OrderedDict here as this is the correct idiom and fix the typing annotations not to depend on the deprecated typing.OrderedDict.

Copy link
Member Author

@NicolasHug NicolasHug Nov 8, 2022

Choose a reason for hiding this comment

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

@datumbox all dicts preserve insertion order as of Python 3.6: https://stackoverflow.com/questions/39980323/are-dictionaries-ordered-in-python-3-6. EDIT: more importantly it is also a guaranteed language feature from 3.7, so it's not just a CPython detail.

I was assuming this would fulfil the requirements you noted for the sequential call. Am I missing something important?

Copy link
Contributor

Choose a reason for hiding this comment

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

PyTorch Sequential accepts an OrderedDict according to the docs. Since Dict can in principle be any implementation of the data structure, it's best not to rely on the order of the keys which might change on the future again. Here is an alternative patch at #6929 which maintains OrderedDict, just fixes the typing info and passes mypy without extra ignores. I'll close that PR as it's meant only for illustration purpose but feel free to cherrypick some of the approach here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since Dict can in principle be any implementation of the data structure

No, it's not an implementation detail anymore in 3.7. It's a guaranteed language feature (see link above). We can rely on it.

As noted below Sequential should be able to handle dicts on top of OrderedDicts now that 3.7 is the minimum required version. It might be a good opportunity to make that change upstream (looks like @pmeier is looking into it)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes dicts are insertion ordered from python 3.7. This is legit feature and used widely.

@@ -96,7 +96,7 @@ def __init__(
else:
self.stochastic_depth = nn.Identity() # type: ignore

_layers = OrderedDict()
_layers = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, by removing the order the Sequential won't work as we expect.

@NicolasHug
Copy link
Member Author

NicolasHug commented Nov 8, 2022

torchvision/models/maxvit.py:125: error: No overload variant of "Sequential"
matches argument type "Dict[str, Module]"  [call-overload]
            self.layers = nn.Sequential(_layers)

Sigh. I won't have too much time to dedicate to this. @pmeier could you please take over this PR to make Sequential happy? The goal here is to remove the use of the offending typing.OrderedDict.

This sounds like a limitation of Sequential BTW. It should accept pure dicts since they're guaranteed to be insertion-ordered. Might be worth fixing that upstream?

@datumbox
Copy link
Contributor

datumbox commented Nov 8, 2022

@pmeier The following PR makes mypy happy without modifying the model: #6929

@pmeier pmeier self-assigned this Nov 10, 2022
@NicolasHug
Copy link
Member Author

Sequential doesn't support dicts for now, so I'll close this PR. Support request is tracked in pytorch/pytorch#84751

@NicolasHug NicolasHug closed this Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants