-
Notifications
You must be signed in to change notification settings - Fork 39
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: Use micromamba to test Python and ROOT #220
ci: Use micromamba to test Python and ROOT #220
Conversation
.github/workflows/tests.yml
Outdated
- root-version: "6.24.00" | ||
python-version: "3.10" |
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.
ROOT v6.24.00
isn't compatible with modern Python.
.github/workflows/tests.yml
Outdated
- root-version: "6.26.10" | ||
python-version: "3.6" | ||
- root-version: "6.26.10" | ||
python-version: "3.7" |
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.
The conda-forge distribution of ROOT v6.26
isn't compatible with Pythons that are EOL or on the verge of EOL.
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #220 +/- ##
==========================================
+ Coverage 87.80% 88.01% +0.20%
==========================================
Files 4 4
Lines 968 968
Branches 189 202 +13
==========================================
+ Hits 850 852 +2
+ Misses 86 85 -1
+ Partials 32 31 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
7508ce5
to
7fd168a
Compare
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.
Thanks once more, this helps a lot! I left a couple of comments in the code. I hope the tests will answer some of my questions 😄
.github/workflows/tests.yml
Outdated
@@ -22,15 +26,40 @@ jobs: | |||
strategy: | |||
matrix: | |||
os: [ubuntu-latest] | |||
python-version: ["3.8"] | |||
root-version: ["6.24.00", "6.26.10"] |
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.
Any reason not to go with the latest release of 6.24, i.e. 6.24.08?
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.
Nope. That's fine to update to, I was just leaving this as it currently exists.
Instead of using ==
for ROOT I can also just use =
and specifying down to the minor version (which with conda is the same as ~= x.y.0
) though ROOT doesn't really follow SemVer and so there is no reason to trust that testing against the latest patch release won't break something for an unclear reason. But maybe that's better to catch anyway as it is better to know in CI that things are broken than finding out later.
7fd168a
to
ca30b85
Compare
* Use the mamba-org/provision-with-micromamba GitHub action to setup a micromamba enviroment with a ROOT version and CPython version defined by the CI matrix. - Set a default shell of a Bash login to automatically source the micromamba environment. * Test ROOT v6.24.x and v6.26.x and all compatible Python versions with them. - Exclude versions of Python that are not compatible with ROOT releases. - Test on macOS with the latest ROOT release on conda-forge (v6.26.x) and the latest supported CPython version (3.10). * Install ImageMagick and Ghostscript through conda-forge. * Remove commented out macOS and Python 2.7 workflows.
ca30b85
to
b110f0a
Compare
environment-name: ci | ||
extra-specs: | | ||
python=${{ matrix.python-version }} | ||
root=${{ matrix.root-version }} |
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.
Using 6.24
and 6.26
along with root=
instead of root==
means that this is asking conda-forge
for something equivalent to ~=6.26.0
in pip
, for 6.26
for example.
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.
That sounds good! Would you mind changing to =
?
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.
It already does use =
.
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.
Sorry, I see it now, was looking at an outdated diff for some reason...
include: | ||
- os: macos-latest | ||
root-version: "6.26" | ||
python-version: "3.10" |
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.
This adds back in macOS tests but just for the latest release, as macos-latest
is much slower given the limited number of runners and high demands that GitHub has.
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.
Thanks again! I'll merge this and will then make a new release so that python 2 can then be deprecated.
environment-name: ci | ||
extra-specs: | | ||
python=${{ matrix.python-version }} | ||
root=${{ matrix.root-version }} |
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.
Sorry, I see it now, was looking at an outdated diff for some reason...
If you want to make a release before removing Python 2 support then let me make a PR that adds a |
Requires #218 to go in first.
📚 Documentation preview 📚: https://hepdata-lib--220.org.readthedocs.build/en/220/