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

Fix tests' broken python version matrix #947

Merged
merged 8 commits into from
Feb 11, 2025

Conversation

mfisher87
Copy link
Collaborator

@mfisher87 mfisher87 commented Feb 10, 2025

Use a separate cache for each python version matrix value.

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--947.org.readthedocs.build/en/947/

@mfisher87 mfisher87 requested a review from chuckwondo February 10, 2025 18:16
Copy link

github-actions bot commented Feb 10, 2025

Binder 👈 Launch a binder notebook on this branch for commit 1d44740

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 8809fb8

Binder 👈 Launch a binder notebook on this branch for commit 6acf86c

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

Binder 👈 Launch a binder notebook on this branch for commit 15418d7

Binder 👈 Launch a binder notebook on this branch for commit 8aa7ce2

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

Binder 👈 Launch a binder notebook on this branch for commit 1cf3750

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

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

Copy link
Collaborator

@chuckwondo chuckwondo left a comment

Choose a reason for hiding this comment

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

Hmmm, I think we're still missing something. Here's what I see in the 3.13 int. tests in the "Run integration tests" step of the workflow:

Run nox -s integration-tests -- --cov=*** --cov-report=term-missing --capture=no --tb=native --log-cli-level=INFO
  nox -s integration-tests -- --cov=*** --cov-report=term-missing --capture=no --tb=native --log-cli-level=INFO
  shell: /usr/bin/bash -e {0}
  env:
    UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
    pythonLocation: /opt/hostedtoolcache/Python/3.11.11/x64
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.11/x64/lib/pkgconfig
    Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.11/x64
    Python[2](https://github.com/nsidc/earthaccess/actions/runs/13247621820/job/36977882662#step:7:2)_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.11/x64
    Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.11/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/[3](https://github.com/nsidc/earthaccess/actions/runs/13247621820/job/36977882662#step:7:3).11.11/x64/lib:/opt/hostedtoolcache/Python/3.13.1/x6[4](https://github.com/nsidc/earthaccess/actions/runs/13247621820/job/36977882662#step:7:4)/lib

Could this be an issue with how we're calling nox itself? Is there something we need to pass to nox to tell it to use the correct python version?

@mfisher87
Copy link
Collaborator Author

mfisher87 commented Feb 10, 2025

This does appear to be a Nox problem. I personally am against using Nox in CI because it introduces an extra layer of indirection to reason about. I'm not sure how to resolve this without removing Nox. Any idea?

@mfisher87
Copy link
Collaborator Author

#948

@jhkennedy
Copy link
Collaborator

I believe this line:
https://github.com/nsidc/earthaccess/blob/main/.github/workflows/integration-test.yml#L114

is setting up nox itself using the matrix python version, which according to the docs should use the same python version for its environments:
https://nox.thea.codes/en/stable/config.html#configuring-a-session-s-virtualenv

But, I think we can just force it by adding:

--python  ${{ matrix.python-version }}

To the nox command:
https://github.com/nsidc/earthaccess/blob/main/.github/workflows/integration-test.yml#L120

@jhkennedy
Copy link
Collaborator

jhkennedy commented Feb 10, 2025

Looks like the wntrblm/nox action isn't respecting the python version at all. Here's it's supposed to use python 3.10 to setup nox, but it uses 3.11
https://github.com/nsidc/earthaccess/actions/runs/13247621820/job/36977880474#step:6:96

@jhkennedy
Copy link
Collaborator

Oof, that action is a complete mess when it comes to the Python version:
image

@mfisher87
Copy link
Collaborator Author

I don't understand why we're doing uv setup and nox setup, so I removed uv. Let's see what happens.

@jhkennedy
Copy link
Collaborator

Also worth reading this: wntrblm/nox#735

I'll circle back around to this later today if it's still open

@mfisher87
Copy link
Collaborator Author

mfisher87 commented Feb 10, 2025

Looks like that's not how this works.

nox > Python version selection caused no sessions to be selected.

This very not maintainable. It requires a significant amount of knowledge of the nooks and crannies of Nox do complex CI things like this. I'd like to remove Nox and go back to running the tests directly in CI using uv. It was working great!

@mfisher87 mfisher87 changed the title Use a different Python environment for each python version Integration tests: Use a different Python environment for each python version Feb 10, 2025
@mfisher87 mfisher87 changed the title Integration tests: Use a different Python environment for each python version Fix integration tests' broken python version matrix Feb 10, 2025
@mfisher87 mfisher87 linked an issue Feb 10, 2025 that may be closed by this pull request
1 task
@mfisher87 mfisher87 marked this pull request as draft February 10, 2025 19:51
@chuckwondo
Copy link
Collaborator

This very not maintainable. It requires a significant amount of knowledge of the nooks and crannies of Nox do complex CI things like this. I'd like to remove Nox and go back to running the tests directly in CI using uv. It was working great!

Sounds good to me.

@jhkennedy
Copy link
Collaborator

This very not maintainable. It requires a significant amount of knowledge of the nooks and crannies of Nox do complex CI things like this. I'd like to remove Nox and go back to running the tests directly in CI using uv. It was working great!

I would pose, that if we can't figure out how to use nox in CI, then it's not really something we should expect our users to figure out. Ergo the dogfooding principle: https://en.wikipedia.org/wiki/Eating_your_own_dog_food

@mfisher87
Copy link
Collaborator Author

mfisher87 commented Feb 10, 2025

Using nox in CI is significantly different than using it as a user -- I don't see any way an end-user can run in to this problem, and I don't think we expect (or want) end-users to be testing with every version of Python unless they are comfortable doing that. We should absolutely be dogfooding Nox locally. That's how I run tests.

@mfisher87
Copy link
Collaborator Author

@chuckwondo @jhkennedy tests are running on the correct version now!

@mfisher87
Copy link
Collaborator Author

Unit tests are also all running on 3.11. Working on that too

@mfisher87 mfisher87 changed the title Fix integration tests' broken python version matrix Fix tests' broken python version matrix Feb 10, 2025
@mfisher87 mfisher87 requested a review from chuckwondo February 10, 2025 22:27
@mfisher87 mfisher87 requested a review from jhkennedy February 10, 2025 22:28
@mfisher87 mfisher87 marked this pull request as ready for review February 10, 2025 22:34
.github/actions/install-pkg/action.yml Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
mfisher87 and others added 2 commits February 10, 2025 16:58
Co-authored-by: Joseph H Kennedy <me@jhkennedy.org>
Co-Authored-By: Joseph H Kennedy <me@jhkennedy.org>
@mfisher87 mfisher87 force-pushed the fix-integration-tests-env-matrix branch from 1cf3750 to b81c2b6 Compare February 11, 2025 00:01
@mfisher87 mfisher87 requested a review from jhkennedy February 11, 2025 00:02
jhkennedy
jhkennedy previously approved these changes Feb 11, 2025
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.

Looks good to me! The CI setup is definitely a lot more straightforward now. 🥇

@mfisher87
Copy link
Collaborator Author

Woops, pushed a commit to the wrong PR, so force-pushed to HEAD~1.

@mfisher87 mfisher87 merged commit d763d9c into main Feb 11, 2025
20 of 24 checks passed
@mfisher87 mfisher87 deleted the fix-integration-tests-env-matrix branch February 11, 2025 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Integration tests are always running against Python 3.11
3 participants