-
Notifications
You must be signed in to change notification settings - Fork 283
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
CI lockfiles #4108
CI lockfiles #4108
Conversation
Comments and cron scheduling for refresh-lockfiles workflow
noxfile now handles out of date environments, so no need to invalidate the cache in the cirrus configuration
You can see an example of the GH Actions run here: https://github.com/jamesp/iris/actions/runs/790009248 And the PR it produces here: jamesp#3 |
@jamesp Did you try using Or would that also struggle in |
We could also rationalise the requirements themselves, as There are options to make this less bloaty, package wise... but maybe that can come later. |
No I didn't but I'm sure that would work too and would be a viable alternative to this setup. But then you are still doing a resolve on every run of the CI. We first need to decide if lockfiles belong in version control. If yes, this scheme provides a mechanism for keeping them up to date. If no, this scheme should be discarded. we could instead keep the lockfiles cached in CI to reduce the number of resolves |
I'm 100% in favour of the former and against the latter. As I said before, this can solve issues beyond CI - benchmarking being the example I'm aware of currently. |
The pro for this approach is that we get a known, reproducible, test environment for every CI run until we explicitly update the lockfiles committed to git. As a workflow, I was thinking somethign along the lines of:
|
@jamesp How will this work for feature branches, that pull in additional or different requirements. Any thoughts? |
@jamesp Interesting that there are graphical Do you have scope and capacity to investigate this further? |
That particular gallery example is missing bits (see #4110). I have no idea if the failure is related to that though. Edit: now thinking it quite likely is that: #4110 (comment) |
Given the output of the test above suggests the code is working, it's the same test that's failing,I would suggest, if everyone is comfortable, we ignore that here, and rebase #4104. I'm not sure about the gallery failure, I'll take a look at the imagehash on 3.8 and see what the issue is. the hamming distance score suggests it's not far off. |
We should address that image hash in a separate PR, I think this one is about complete now. |
What more do we need to do to get comfortable with merging this with the failing doc tests? We seem to be approaching a circular dependency across a number of PRs! |
@pp-mo also suggested we should briefly discuss this as a team. It's not just a matter of "does it work?" - it's a change in how we do things. |
@jamesp don't get me wrong, I'm also very keen we get this over the line before we get an even bigger spider web of blocking problems. |
Picked from SciTools@566f187
* 'ci-lock' of github.com:jamesp/iris: Apply suggestions from @trexfeathers code review
Thanks @trexfeathers. Pushed commits to address those points above. I agree this is a change to how things are done that should be discussed. Could @trexfeathers or @pp-mo setup a quick call for all those interested? |
That anomaly log gallery failure also came up in my local testing but I ignored it because it wasn't previously failing here. The idiff output looked similar to the change shown at SciTools/test-iris-imagehash#40, except the colour bar was also affected. For the square root failure, see #3993. 😔 |
I've reverted the changes to Once this is merged as a priority we should fix:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm this only includes the expected CI failures. Merging as agreed.
* master: (49 commits) Update CI environment lockfiles + Cartopy 0.19 changes (SciTools#4125) separate arg types from descriptions (SciTools#4100) Use assertArrayAllClose for sqrt test (SciTools#4118) Removed branch suffix (SciTools#4117) Corrected plot_anomaly_log_colouring for new Matplotlib linscale rules. (SciTools#4115) replace most recent hashes (SciTools#4112) Linkcheck update (SciTools#4104) CI lockfiles (SciTools#4108) Fix bug in gallery_tests runner (SciTools#4111) add logo to conda and pypi badges (SciTools#4088) pre-commit-ci update (SciTools#4092) Fix cirrus ci and mpl (SciTools#4087) Update readme docs stable (SciTools#4089) linux kernel version fix for conda 4.10+ (SciTools#4084) update docs pypi instructions (SciTools#4077) update flake8 pre-commit (SciTools#4067) Add GitHub discussions badge (SciTools#4070) conda requirements add pip (SciTools#4062) add pre-commit.ci badge and doc support (SciTools#4061) cirrus-ci credits for non-master (SciTools#4060) ...
* master: (87 commits) Contourf Antialias Fix (SciTools#4150) minor tidy of cirrus and nox (SciTools#4152) update bug and feature templates (SciTools#4147) Update CI environment lockfiles + Cartopy 0.19 changes (SciTools#4125) separate arg types from descriptions (SciTools#4100) Use assertArrayAllClose for sqrt test (SciTools#4118) Removed branch suffix (SciTools#4117) Corrected plot_anomaly_log_colouring for new Matplotlib linscale rules. (SciTools#4115) replace most recent hashes (SciTools#4112) Linkcheck update (SciTools#4104) CI lockfiles (SciTools#4108) Fix bug in gallery_tests runner (SciTools#4111) add logo to conda and pypi badges (SciTools#4088) pre-commit-ci update (SciTools#4092) Fix cirrus ci and mpl (SciTools#4087) Update readme docs stable (SciTools#4089) linux kernel version fix for conda 4.10+ (SciTools#4084) update docs pypi instructions (SciTools#4077) update flake8 pre-commit (SciTools#4067) Add GitHub discussions badge (SciTools#4070) ...
🚀 Pull Request
Description
Here's a proof of concept for adding locked dependencies and using them in the nox testing framework.
requirements/ci/nox.lock
. These are explicit conda environment files which means they do not require resolving, they are the resolved manifestation of the equivalentrequirements/ci/py3X.yml
. This means that they can be installed usingconda create -n my_new_env --file requirements/ci/nox.lock/py3X-linux-64.lock
.noxfile.py
updated to use these resolved environments and avoid rebuilding / updating until the lockfile changes. This should dramatically reduce the second time the nox test suite runs. It should probably also greatly reduce the time the first run takes too, as there is no need for conda to resolve or update the environment.There's quite a lot in this, so whoever feels like reviewing, lets go through it together 👍
This should resolve #4105
Consult Iris pull request check list