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

Switch to tox #5184

Closed
wants to merge 1 commit into from
Closed

Switch to tox #5184

wants to merge 1 commit into from

Conversation

lbdreyer
Copy link
Member

@lbdreyer lbdreyer commented Mar 1, 2023

WIP This is now ready for review

This switches our use of nox to tox, re #4806

tox.ini Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
.github/workflows/ci-tests.yml Outdated Show resolved Hide resolved
@lbdreyer lbdreyer marked this pull request as ready for review March 31, 2023 14:50
tox.ini Show resolved Hide resolved
tox.ini Show resolved Hide resolved
benchmarks/bm_runner.py Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@lbdreyer lbdreyer force-pushed the tox branch 2 times, most recently from 83219a3 to fd8afb6 Compare March 31, 2023 16:03
@lbdreyer
Copy link
Member Author

So this should be all good to go, but there are now three test failures:

FAILED lib/iris/tests/unit/analysis/stats/test_pearsonr.py::Test::test_perfect_corr
FAILED lib/iris/tests/integration/plot/test_vector_plots.py::TestBarbs::test_2d_plain_latlon
FAILED lib/iris/tests/integration/plot/test_vector_plots.py::TestBarbs::test_2d_plain_latlon_on_polar_map
==== 3 failed, 6767 passed, 36 skipped, 15220 warnings in 178.70s (0:02:58) ====

Which is interesting since I've not touched any test files. Maybe something to do with the environment creation!

Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

This is coming together nicely! Couple of final observations.

benchmarks/bm_runner.py Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@lbdreyer lbdreyer force-pushed the tox branch 3 times, most recently from 5752267 to 4d405e1 Compare July 19, 2023 14:31
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.37%. Comparing base (5b42f47) to head (fba88d7).
Report is 417 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5184   +/-   ##
=======================================
  Coverage   89.37%   89.37%           
=======================================
  Files          89       89           
  Lines       22426    22426           
  Branches     5379     5379           
=======================================
  Hits        20043    20043           
  Misses       1637     1637           
  Partials      746      746           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lbdreyer
Copy link
Member Author

This is getting closer. I just have to make change to the benchmarks and test them locally.

@lbdreyer lbdreyer force-pushed the tox branch 2 times, most recently from 69e92f8 to 6b34302 Compare July 20, 2023 14:23
@lbdreyer
Copy link
Member Author

Benchmarks ran, so this should now be ready!

@trexfeathers
Copy link
Contributor

Since everything is in a single commit I think I'll need to go back through all the changes - I can't remember which bits I did and didn't review back in March.

So this will need to wait until after the sprint.

@lbdreyer
Copy link
Member Author

Thanks! no rush at all!

@trexfeathers trexfeathers self-assigned this Jul 26, 2023
Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

OK, I've been through this with fresh eyes, including lots of trial tox runs locally, and I think it makes sense. The Tox world seems a lot slicker than the Nox world, even if I did need to learn a new syntax 👍

So my only outstanding caution is concerning dependency on tox-conda, since it's such a small project. See below.

cache_period: ${{ env.CACHE_PERIOD }}
env_name: ${{ env.ENV_NAME }}
install_packages: "cartopy nox pip"
install_packages: "cartopy tox'<4'"
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel a little uncomfortable that this remains necessary over 6 months later (see tox-dev/tox-conda#163).

tox-conda looks in need of more maintainers, are we comfortable being dependent on it? @lbdreyer @bjlittle

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need Nox OR Tox anymore, now that we use lock files?!

Having thought more, that was ill-informed, since Tox still offers neat inheritance tricks. But if we stick with tox-conda, we need to be comfortable sticking with the pin as well. Of course being pinned kinda undermines the idea of adopting something more popular.

Thanks @ESadek-MO and @HGWright for conversation on this topic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also leaves us to potential security vulnerabilities:

tox-dev/tox#2524 (comment)

@trexfeathers trexfeathers mentioned this pull request Nov 29, 2023
3 tasks
@trexfeathers
Copy link
Contributor

Looks like this didn't get recorded, but discussions between @SciTools/peloton in December concluded that we should investigate how easy it would be to maintain our own tox-conda plugin, with just the features that we need. @pp-mo expressed an interest in this, I don't know how it has progressed.

We accepted that this is less good than maintaining the real, public plugin. But we also agreed that it could be the pragmatic approach since a private plugin would take less maintenance and make us less vulnerable to outside forces.

@HGWright highlighted that if we got a proof-of-concept together then we could potentially start a conversation on the tox-conda repo to see how many others are maintaining their own plugin and discuss pooling resources.

This matters for this PR, as I am increasingly reluctant to approve tox-conda becoming a dependency.

@trexfeathers trexfeathers removed their assignment Feb 21, 2024
@trexfeathers trexfeathers added the Dragon 🐉 https://github.com/orgs/SciTools/projects/19?pane=info label Feb 21, 2024
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@lbdreyer lbdreyer closed this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dragon 🐉 https://github.com/orgs/SciTools/projects/19?pane=info
Projects
Status: 💰 Finished
Status: 📋 Backlog
Status: 🏁 Done
Development

Successfully merging this pull request may close these issues.

4 participants