Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

fix imports of OrderedDict from typing to collections #5481

Closed
wants to merge 8 commits into from

Conversation

ethanjyx
Copy link

Changes proposed in this pull request:

Remove any imports of OrderedDict from typing module.
OrderedDict is not included into typing until Python 3.7.2, at my company right now we are using Python 3.6.9 and this is causing issues for us.
Also OrderedDict from typing is deprecated later since Python 3.9. Reference: https://docs.python.org/3/library/typing.html#typing.OrderedDict.

So importing OrderedDict from typing is not a good idea, and we change that to import from collections here.

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

After submitting

  • All GitHub Actions jobs for my pull request have passed.
  • codecov/patch reports high test coverage (at least 90%).
    You can find this under the "Actions" tab of the pull request once the other checks have finished.

Sorry, something went wrong.

@@ -27,7 +28,7 @@
from allennlp.models import Model


StateDictType = Union[Dict[str, torch.Tensor], OrderedDict[str, torch.Tensor]]
StateDictType = Union[Dict[str, torch.Tensor], Mapping[str, torch.Tensor]]
Copy link
Author

Choose a reason for hiding this comment

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

I had to change this to Mapping because with OrderedDict from collections I got TypeError: 'type' object is not subscriptable

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, have you tried quoting OrderedDict? Like

Suggested change
StateDictType = Union[Dict[str, torch.Tensor], Mapping[str, torch.Tensor]]
StateDictType = Union[Dict[str, torch.Tensor], "OrderedDict[str, torch.Tensor]"]

Copy link
Author

Choose a reason for hiding this comment

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

Sorry - haven't got time to respond and test. @epwalsh are you going to take this over? Saw you self-assigned.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @ethanjyx, unfortunately we dropped support for 3.6 as of #5400.

Copy link
Author

Choose a reason for hiding this comment

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

@epwalsh thanks for letting me know that, that's helpful.

But you still might want to consider doing this PR, since OrderedDict from typing is deprecated since Python 3.9 and this code will break.

Copy link
Member

Choose a reason for hiding this comment

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

Good point! Well if you'd like to finish it up I'm happy to reopen it then

Copy link
Author

Choose a reason for hiding this comment

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

I'll finish it up in January. Pls reopen. Thanks

Copy link
Author

Choose a reason for hiding this comment

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

@epwalsh looks like your suggested approach did not pass lint.. any ideas? not familiar with types within quotes

@epwalsh epwalsh self-assigned this Dec 3, 2021
@epwalsh epwalsh removed their assignment Dec 23, 2021
@epwalsh epwalsh closed this Dec 23, 2021
@epwalsh epwalsh reopened this Jan 4, 2022
@dirkgr
Copy link
Member

dirkgr commented Jan 12, 2022

@ethanjyx, are you working on this again?

@ethanjyx
Copy link
Author

@dirkgr this slipped through - do you have time to continue it?

@ethanjyx
Copy link
Author

@epwalsh @dirkgr I believe I fixed this PR - just waiting the CI, can you pls approve the running workflows?

@dirkgr
Copy link
Member

dirkgr commented Jan 13, 2022

No problem. It turns out I don't have permission to fix the merge conflicts. Should be easy to resolve though.

@ethanjyx
Copy link
Author

ping @epwalsh

@dirkgr
Copy link
Member

dirkgr commented Jan 14, 2022

You can run those checks locally:

make format
make flake8
make typecheck

It looks to me like there is just one import missing.

@ethanjyx
Copy link
Author

@epwalsh can you kick the CI off? thanks

@epwalsh
Copy link
Member

epwalsh commented Jan 18, 2022

Hi @ethanjyx, you'll need to fix the merge conflicts before CI can run

@epwalsh
Copy link
Member

epwalsh commented Mar 24, 2022

Closing since this PR is stale and we don't officially support 3.6 anymore, anyway.

@epwalsh epwalsh closed this Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants