Skip to content

Use dict instead of OrderedDict in maxvit #6927

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Use dict instead of OrderedDict in maxvit #6927

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions torchvision/models/maxvit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import math
from functools import partial
from typing import Any, Callable, List, Optional, OrderedDict, Sequence, Tuple
from typing import Any, Callable, Dict, List, Optional, Sequence, Tuple

import numpy as np
import torch
Expand Down Expand Up @@ -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.

_layers["pre_norm"] = norm_layer(in_channels)
_layers["conv_a"] = Conv2dNormActivation(
in_channels,
Expand Down Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.


# convolutional layer
layers["MBconv"] = MBConv(
Expand Down