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 support for Python 3.9; eliminate duplicate run of test-mindeps CI #876

Merged
merged 7 commits into from
Nov 9, 2024

Conversation

mfisher87
Copy link
Collaborator

@mfisher87 mfisher87 commented Nov 8, 2024

Following SPEC0, we can stop supporting Python 3.9. We can also stop supporting 3.10, but we decided to give 3.10 a little more time before dropping. Resolves #836.

Pull Request (PR) draft checklist - click to expand
  • Please review our
    contributing documentation
    before getting started.
  • Populate a descriptive title. For example, instead of "Updated README.md", use a
    title such as "Add testing details to the contributor section of the README".
    Example PRs: #763
  • Populate the body of the pull request with:
  • Update CHANGELOG.md with details about your change in a section titled
    ## Unreleased. If such a section does not exist, please create one. Follow
    Common Changelog for your additions.
    Example PRs: #763
  • Update the documentation and/or the README.md with details of changes to the
    earthaccess interface, if any. Consider new environment variables, function names,
    decorators, etc.

Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!

Pull Request (PR) merge checklist - click to expand

Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the @nsidc/earthaccess-support team in a comment and we
will help you out!

  • Add unit tests for any new features.
  • Apply formatting and linting autofixes. You can add a GitHub comment in this Pull
    Request containing "pre-commit.ci autofix" to automate this.
  • Ensure all automated PR checks (seen at the bottom of the "conversation" tab) pass.
  • Get at least one approving review.

📚 Documentation preview 📚: https://earthaccess--876.org.readthedocs.build/en/876/

Copy link

github-actions bot commented Nov 8, 2024

Binder 👈 Launch a binder notebook on this branch for commit c782c52

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 04d30a8

Binder 👈 Launch a binder notebook on this branch for commit df3250e

Binder 👈 Launch a binder notebook on this branch for commit 9bbdd73

Binder 👈 Launch a binder notebook on this branch for commit 75845c3

Binder 👈 Launch a binder notebook on this branch for commit a327528

Binder 👈 Launch a binder notebook on this branch for commit a2a4837

@mfisher87 mfisher87 marked this pull request as ready for review November 8, 2024 18:11
@mfisher87
Copy link
Collaborator Author

  Caused by: Failed to download and build `h5py==3.0.0`
  <...>
  Caused by: Failed to download and build `numpy==1.19.3`

?? Our minimum supported version of numpy is 1.24.0. Is uv failing to understand our conditional numpy dependency?

Remove numpy conditional dependency since h5py is incompatible with numpy<1.26.4
@mfisher87
Copy link
Collaborator Author

Removed the numpy conditional dependency after upgrading h5py to a version that supports Python 3.10. Still getting the same error. When I lock the environment locally with lowest-direct resolution, I get: Updated numpy v2.0.2 -> v1.26.4. I don't understand where numpy==1.19.3 is coming from!

@mfisher87
Copy link
Collaborator Author

It looks to me like a bug in uv's lowest-direct resolution algorithm. It's pulling in the lowest version for numpy as an indirect dependency of h5py (https://github.com/h5py/h5py/blob/3.4.0/setup.py#L32) and ignoring our direct specification of the lowest numpy version.

@jhkennedy
Copy link
Collaborator

@mfisher87 this is dumb, but might just work -- can you try putting numpy at the top of the dependencies list so it's before h5py?

@mfisher87 mfisher87 added the help wanted Extra attention is needed label Nov 8, 2024
@mfisher87
Copy link
Collaborator Author

@jhkennedy great idea, but no change :(

@mfisher87 mfisher87 changed the title Drop support for Python 3.9 Drop support for Python 3.9; eliminate duplicate run of test-mindeps CI Nov 8, 2024
@jhkennedy
Copy link
Collaborator

OK, rats. I was hoping dumb but easy would work. 🤣

@mfisher87 in #872, when I pulled the min-deps test into nox, I changed:

uv sync --resolution lowest-direct --extra test
uv pip install --no-deps .

to

uv pip install --resolution lowest-direct --editable .[test]

https://github.com/nsidc/earthaccess/pull/872/files#diff-f7a16a65f061822bcc73b8296f4dc837353d379d8d9cc5307982cb6941442835R38

It sounds like from your comments the uv sync step first might actually be necessary, so adding that back into the session might be the way to go.

@jhkennedy
Copy link
Collaborator

@mfisher87
Copy link
Collaborator Author

The uv sync command fails with the exact same error output :(

@jhkennedy
Copy link
Collaborator

(╯°□°)╯︵ ┻━┻

@mfisher87
Copy link
Collaborator Author

I'm thinking open an issue on the uv repo for this. What do you think?

@jhkennedy
Copy link
Collaborator

I'm thinking open an issue on the uv repo for this. What do you think?

Makes sense -- there's a couple that look like they might be related already open. E.g.,
astral-sh/uv#5492

But I'm in a meeting so haven't ready them closely (I'm all booked today 😭 )

@mfisher87
Copy link
Collaborator Author

mfisher87 commented Nov 8, 2024

I never did figure out the underlying cause. pip install h5py==3.4.0 doesn't work in a Python 3.10 environment either; it attempts to build the wheel, and pulls in the lowest supported version of numpy and fails with the same message as we receive in CI. I expected it would pull in a version of numpy which supported Python 3.10.

Either way, we can avoid building the wheel by upgrading h5py >=3.6.0, which has a pre-built wheel for Python 3.10, and that fixes the error!

Copy link
Collaborator

@jhkennedy jhkennedy left a comment

Choose a reason for hiding this comment

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

The 3.10 and 3.11 integration tests failed because 6 tests failed, while in 3.12 and 3.13 only 5 failed, which is below our threshold. The additional failure is just a flakey failure, so I think were g2g. :shipit:

@mfisher87
Copy link
Collaborator Author

🤣

@mfisher87 mfisher87 merged commit ba7363b into main Nov 9, 2024
13 checks passed
@mfisher87 mfisher87 deleted the drop-python-3.9-support branch November 9, 2024 01:00
Copy link

User ayushnag does not have permission to run integration tests. A maintainer must perform a security review of the code changes in this pull request and re-run the failed integration tests jobs, if the code is deemed safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop support for Python 3.9
2 participants