-
Notifications
You must be signed in to change notification settings - Fork 91
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 overhaul #2298
base: stable
Are you sure you want to change the base?
CI overhaul #2298
Conversation
What the heck is pixi? This https://pixi.sh/latest/ ? Is this mature/stable? It doesn't have a wikipedia page and I have never heard from it... |
Yes, that link is correct. It is a relatively new package manager, it was released in summer last year (they had a release party at EuroSciPy 2023, Reimar and I attended it). It is developed by the same people that were/are behind mamba, so for what it's worth there is some credibility behind it.
I haven't worked with cargo/rust in a while but yes, I think they took a lot of inspiration from that and there are many similarities in how they work.
Hard agree.
Yes, the mss-feedstock and therefore the conda-forge package remains the same (and installable through conda, mamba, pixi or whatever else might work with conda-forge packages, with the usual caveat that conda-forge packages can not be tested and might break at any point in time due to the install-time dependency resolution - nothing new from this PR and a gripe I have with the entire conda packaging model). |
I recently gave a lighning talk at the Barcamp in Karlsruhe. Sources: There is also a talk by Wolf on pyconde in mid april |
with an uptodate conda you have same speed as with mamba, see |
Speed isn't the issue that conda has, in my opinion. The issue is that it is impossible to define a development environment in its entirety right next to the source code (i.e. inside of a git repository). Mamba has the same issue, because its interface is basically the same as conda's. Cargo fixes this for the rust world with its Cargo.lock file pinning dependencies from crates.io, PDM or poetry (or others) can do the same for python on top of PyPI, nix can do it independently of the programming language using pinned references to nixpkgs, and pixi does it on top of the conda packaging ecosystem (also basically language independent). Conda or mamba simply don't provide this feature, speed is irrelevant in that case. (Although, using a lock file moves the dependency resolution to the step of creating the lock file, so that at install time there is no dependency resolution at all necessary. A no-op is strictly faster than any solver conda could possibly integrate.) |
With trying to differentiate the packages #2390 I will likly get to a conda requirements.d/mswms.txt, requirements.d/mscolab.txt and likly a requirements.txt for the whole. I am currently looking for the jinja syntax to read a requirements.txt files into the current meta.yaml which I prefer to use until we can migrate to e.g. rattler-build etc. on conda-forge too. The one in the feedstock is not identical because of the cross compilation. Without any extra hassle I want to be able to redo a build by conda build locally. But keeiping that dir is then independent for switching in a first step to a requirements.txt instead of current transformation from a meta.yaml. With the jinja2 Syntax there is then also no duplication. |
This removes the docker image logic from the CI setup and instead replaces it with a setup based on pixi. As part of that it moves the dependency specification from localbuild/meta.yaml to pixi.toml and pixi.lock. This turns the MSS repository into a single source of truth for both the application code as well as the development environment (whereas the latter was previously only specified in the docker images, and not reproducible in any way). Setting up a development environment is as simple as installing pixi and running `pixi shell` (or `pixi run <cmd>`, or `pixi install` to just create the environment, etc.). This environment will, by construction, be the same that is used in the CI as well (modulo platform differences). There is a new workflow that periodically (once a week on Monday) recreates the pixi lockfile and opens a PR for that update. The checks in that PR essentially serve as a replacement for the previous scheduled runs to ensure that no dependency update breaks MSS. Merging that PR is a manual step that can be done just as with any other PR and would then update the environment on the given target branch. This is essentially what was previously the triggering of a docker image creation. Including new dependencies can be done with `pixi add`, which will also automatically add the dependency to the (pre-existing) lockfile. This means dependency additions can be part of the PR that necessitate them and they won't affect the entire environment (as they previously did, where they would trigger a full image rebuild) but instead just add that new package to the existing specification.
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.
nice!
Wolf recently announced a speedup using conda-forge from prefix-dev.
https://x.com/wolfvollprecht/status/1863590394992193803
We maybe should move all docs etc to pixi too, there is also an issue for that.
pixi.toml
Outdated
scipy = "*" | ||
shapely = ">=2.0.0" | ||
skyfield = ">=1.12" | ||
skyfield-data = ">=5" |
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.
>=6
when it not always uses the newest, we would need to enumber it each year
pixi.toml
Outdated
libxmlsec1 = "*" | ||
|
||
[feature.dev.dependencies] | ||
boa = "*" |
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.
we should remove this.
This was also only an intermediate on the way to rattler-build
sphinx = "*" | ||
sphinx_rtd_theme = "*" | ||
xmlschema = "<2.5.0" | ||
|
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 sections are a nice feature, so we could also add a section for the tutorials.
pixi.toml
Outdated
pytest-xdist = "*" | ||
sphinx = "*" | ||
sphinx_rtd_theme = "*" | ||
xmlschema = "<2.5.0" |
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 xmlschema and its pinning was removed some time ago
https://github.com/Open-MSS/MSS/pull/2117/files
pixi.toml
Outdated
lxml = "*" | ||
markdown = "*" | ||
matplotlib = ">=3.5.3" | ||
menuinst = "*" |
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.
menuinst is not implemented for pixi, prefix-dev/pixi#1894
@ReimarBauer this is still WIP, I have only rebased so far and didn't yet update the pixi.toml file to reflect all of the changes since end of March or work on the documentation or anything. I am also looking into |
I know that it has WIP state, but I like it especially because of the recent problems which perfectly illustrate what disadvantages we have if we don't do it. These are benevolent comments ;) |
This removes the docker image logic from the CI setup and instead replaces it with a setup based on pixi. As part of that it moves the dependency specification from localbuild/meta.yaml to pixi.toml and pixi.lock. This turns the MSS repository into a single source of truth for both the application code as well as the development environment (whereas the latter was previously only specified in the docker images, and not reproducible in any way).
Setting up a development environment is as simple as installing pixi and running
pixi shell
(orpixi run <cmd>
, orpixi install
to just create the environment, etc.). This environment will, by construction, be the same that is used in the CI as well (modulo platform differences).There is a new workflow that periodically (once a week on Monday) recreates the pixi lockfile and opens a PR for that update. The checks in that PR essentially serve as a replacement for the previous scheduled runs to ensure that no dependency update breaks MSS. Merging that PR is a manual step that can be done just as with any other PR and would then update the environment on the given target branch. This is essentially what was previously the (manual) trigger of a docker image creation.
Including new dependencies can be done with
pixi add
, which will also automatically add the dependency to the (pre-existing) lockfile. This means dependency additions can be part of the PR that necessitate them and they won't affect the entire environment (as they previously did, where they would trigger a full image rebuild) but instead just add that new package to the existing specification.This (mostly) implements the ideas I've outlined in #2160. I wanted to substantiate what I had in mind, because I think there was some confusion there about what these ideas would entail.
I consider this to be a massive simplification of the CI setup, while retaining all required functionality (or even improving it). The additions count looks large due to the pixi.lock file, but that file is automatically generated. Outside of it this is a net negative in code size.
There is the minor issue that PRs created from GitHub Actions do not trigger workflow runs (an arbitrary and annoying limitation of GitHub Actions, if you ask me), so the checks in the lockfile update PR that I mentioned above wouldn't actually be created (yet). There are multiple workarounds available, the easiest being closing and re-opening the PR (and more are documented here: https://github.com/peter-evans/create-pull-request/blob/main/docs/concepts-guidelines.md#triggering-further-workflow-runs).
Of course, there would need to be documentation changes as well. This is a draft and I would like to discuss if this is a direction we want to go in before fleshing this out more.
@ReimarBauer @joernu76 just pinging you so you are aware of it, there is no sense of urgency with this.