Skip to content
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.

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

Remove usages of legacy importlib API. #2531

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bwoodsend
Copy link

Motivation

The Python >= 3.10 compatible usages of importlib added in 277ff54 can be safely applied to any version of Python >= 3.5 which covers the full range that Hydra supports.

Getting rid of the old deprecated API, asides from cleaning up, will keep Hydra compatible with PyInstaller if PyInstaller doesn't get cold feet and revert its dropping of support for the legacy API (pyinstaller/pyinstaller#7344).

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

(How should this PR be tested? Do you require special setup to run the test or repro the fixed bug?)

Standard CI/CD testing should be sufficient.

Related Issues and PRs

(Is this PR part of a group of changes? Link the other relevant PRs and Issues here. Use https://help.github.com/en/articles/closing-issues-using-keywords for help on GitHub syntax)

None.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 3, 2023
@bwoodsend bwoodsend force-pushed the remove-old-importlib branch 2 times, most recently from 09cfab0 to b462e93 Compare January 4, 2023 22:35
@bwoodsend
Copy link
Author

Remaining failures look sufficiently irrelevant to call background noise.

@bwoodsend
Copy link
Author

@Jasha10 ?

@Jasha10
Copy link
Collaborator

Jasha10 commented Jan 11, 2023

Thanks for the ping @bwoodsend! We should be able to merge this shortly.

@omry
Copy link
Collaborator

omry commented Jan 11, 2023

Any concerns about compatibility with supported legacy Python versions?

@bwoodsend
Copy link
Author

No, because current hydra doesn't support Python 3.5.

Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

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

Thanks again @bwoodsend.
One relatively minor nit / question:

hydra/core/plugins.py Show resolved Hide resolved
@omry
Copy link
Collaborator

omry commented Jan 12, 2023

No, because current hydra doesn't support Python 3.5.

This code is running on 3.6 to 3.9, wasn't it?

                        if sys.version_info < (3, 10):
                            m = importer.find_module(modname)  # type: ignore

@Jasha10
Copy link
Collaborator

Jasha10 commented Jan 12, 2023

This code is running on 3.6 to 3.9, wasn't it?

Yes. I introduced that if < (3, 10)-block in PR #2183 because the code was working for 3.6-3.9 but it did not work for python3.10.

@omry
Copy link
Collaborator

omry commented Jan 12, 2023

This code is running on 3.6 to 3.9, wasn't it?

Yes. I introduced that if < (3, 10)-block in PR #2183 because the code was working for 3.6-3.9 but it did not work for python3.10.

Gotcha. I did seem unfamiliar :)

The Python >= 3.10 compatible usages of importlib added in
277ff54 can be safely applied to any
version of Python >= 3.5 which covers the full range that Hydra
supports.

Getting rid of the old decrecated API, asides from cleaning up, will
keep Hydra compatible with PyInstaller if PyInstaller doesn't get cold
feet and revert its dropping of support for the legacy API
(pyinstaller/pyinstaller#7344).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants