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

drop py3.7 & fix CI #2854

Merged
merged 8 commits into from
Feb 29, 2024
Merged

drop py3.7 & fix CI #2854

merged 8 commits into from
Feb 29, 2024

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Feb 22, 2024

Drop support for python3.7 and fix CI failures

This PR makes the following changes:

  • configen: use omegaconf._utils.type_str directly instead of vendoring type_str
  • drop python3.6 & python3.7 support by updating version pins, config files, etc.
  • fix some linter issues: black, mypy
  • fix some pytest failures related to handling of UserWarning's. I believe these failures surfaced when pytest 8.0 was released.

Reviewers may wish to view commits one-by-one.

Closes #2852.

Previously, the `configen` utility had a vendored version
of the `omegaconf._utils.type_str` function.
Originally, the behavioral differences between the vendored
`configen.utils.type_str` function and omegaconf's `type_str`
were minimal, differing only on an ellipsis input `...`.
Later, the two functions converged in this regard, as
`omegaconf._utils.type_str` added support for ellipsis input.
Given that the two functions converged, we can drop the
vendored version of `type_str` from `configen` and instead
use `omegaconf._utils.type_str` directly.
@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 Feb 22, 2024
@Jasha10 Jasha10 mentioned this pull request Feb 22, 2024
@Jasha10
Copy link
Collaborator Author

Jasha10 commented Feb 22, 2024

This PR supersedes PR #2853.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Feb 22, 2024

CI failing due to unhandled UserWarnings, issue #2855. CI is otherwise clean.
Edit: fixed in commit d4009b4.

@Jasha10 Jasha10 changed the title drop py3.7 drop py3.7 & fix CI Feb 24, 2024
Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot for tackling this!

Just one comment besides the small ones below: would it be possible to use the same versions of the linter libraries as OmegaConf in requirements/dev.txt, and use that for formatting? (if needed you can upgrade those in OmegaConf as well). This will make things easier and more consistent for developers.

hydra/experimental/initialize.py Show resolved Hide resolved
news/2852.api_change Outdated Show resolved Hide resolved
@Jasha10
Copy link
Collaborator Author

Jasha10 commented Feb 27, 2024

would it be possible to use the same versions of the linter libraries as OmegaConf in requirements/dev.txt, and use that for formatting?

Sure, I've updated pins on the linters in commit 08b6912 and have opened a corresponding OmegaConf PR omry/omegaconf#1159.

Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@Jasha10 Jasha10 merged commit 4db334b into facebookresearch:main Feb 29, 2024
25 checks passed
@Jasha10 Jasha10 deleted the drop-py3.7 branch February 29, 2024 04:28
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.

Drop python3.7 support
3 participants