Skip to content

Conversation

adrinjalali
Copy link
Member

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.

@adrinjalali
Copy link
Member Author

cc @TamaraAtanasoska

with:
python-version: ${{ matrix.python }}
pixi-version: v0.38.0
Copy link
Contributor

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
Copy link
Contributor

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

@TamaraAtanasoska
Copy link
Contributor

@adrinjalali is this something fairlearn could benefit from as well, especially since we are doing the compatibility work there too?

Copy link
Contributor

@glemaitre glemaitre left a 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.

@adrinjalali
Copy link
Member Author

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.

@BenjaminBossan
Copy link
Collaborator

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.

@adrinjalali
Copy link
Member Author

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).

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.

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.

Yeah this is quite easy now, we just add sections at the end of the pyproject.toml file and add it to the CI.

@BenjaminBossan
Copy link
Collaborator

I see, sounds like a nice improvement.

@adrinjalali
Copy link
Member Author

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.

@adrinjalali adrinjalali merged commit c98560e into skops-dev:main Dec 3, 2024
4 of 10 checks passed
@adrinjalali adrinjalali deleted the pyproject/build branch December 3, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants