-
Notifications
You must be signed in to change notification settings - Fork 53
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
MNT moving to pixi for better CI reprodocibility #451
Conversation
.github/workflows/build-test.yml
Outdated
with: | ||
python-version: ${{ matrix.python }} | ||
pixi-version: v0.38.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.
You have 0.39 nowadays :)
@@ -52,55 +43,21 @@ jobs: | |||
echo PR_COMMIT_MESSAGE=$(git log -1 --pretty=format:\"%s\") >> $GITHUB_ENV | |||
shell: bash | |||
|
|||
- name: Set up Python ${{ matrix.python }} | |||
uses: actions/setup-python@v5 | |||
- uses: prefix-dev/setup-pixi@v0.8.1 |
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.
In a next iterations, you can add something like this actions: https://github.com/skrub-data/
It allows to run the pixi update
via an Actions and open a pull-request
@adrinjalali is this something fairlearn could benefit from as well, especially since we are doing the compatibility work there too? |
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 looks good to me. The handling of the upper bound using pypi
is a bit tricky but I'm not sure that you can do better.
The issue right now is that we only set sklearn versions and not scipy/numpy/pandas, and that fails with such errors for sklearn=1.1 for instance: ERROR skops/hub_utils/tests/test_hf_hub.py::TestAddFiles::test_adding_str_and_path[pickle] - DeprecationWarning: is_sparse is deprecated and will be removed in a future version. Check `isinstance(dtype, pd.SparseDtype)` instead.
ERROR skops/hub_utils/tests/test_hf_hub.py::TestAddFiles::test_file_does_not_exist_raises[pickle] - DeprecationWarning: is_sparse is deprecated and will be removed in a future version. Check `isinstance(dtype, pd.SparseDtype)` instead.
ERROR skops/hub_utils/tests/test_hf_hub.py::TestAddFiles::test_adding_existing_file_works_if_exist_ok[pickle] - DeprecationWarning: is_sparse is deprecated and will be removed in a future version. Check `isinstance(dtype, pd.SparseDtype)` instead.
ERROR skops/hub_utils/tests/test_hf_hub.py::TestAddFiles::test_adding_existing_file_raises[pickle] - DeprecationWarning: is_sparse is deprecated and will be removed in a future version. Check `isinstance(dtype, pd.SparseDtype)` instead. We'd need to set versions for those dependency packages as well in each task. I'm going to remove old ones here, get a working CI for recent releases, and we can add support for older ones in separate PRs. |
I haven't worked with pixi, but heard good things. If it simplifies the CI, I'm all for it (though I'm not sure what the point of the lock file is). I think one good measurement of if/how much this improves the CI is: How difficult is it to adjust if a new sklearn version or Python version is released. |
It's where all the specs for environments are, which makes things reproducible. We've also moved to using conda lock files in sklearn for a while and a bot updates that automatically, and it's been making life much easier since you have full control over what happens on the CI.
Yeah this is quite easy now, we just add sections at the end of the |
I see, sounds like a nice improvement. |
So the CI is now according to expectations, passing only on 1.5 (latest release). The 1.6 and the nightly (1.7) should also start passing once they're released / the nightly build for sklearn is fixed. I'll work on the fixes for more versions right after. |
Moving to pixi since we actually were not testing older sklearn versions, and this makes it very easy to test things the same way as things are in the CI.
I'll need to add some docs in contributing guides.
cc @BenjaminBossan WDYT?
cc @glemaitre, would be nice if you could have a look. CI shouldn't be green on this, failures are expected.