-
Notifications
You must be signed in to change notification settings - Fork 40
Manage dependencies with pixi #407
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
Conversation
At the risk of being really annoying, should we be switching to pixi or to uv here? |
It doesn't matter to me as long as we find a way to install all dependencies without using conda environment files. Pip installing h5py doesn't install the netcdf4/hdf5 C libraries which will give the user a module not found error when trying to use those readers. So, in this PR I use the conda distributions of those libraries in Lines 121 to 124 in a2b5461
|
Thanks for the clarification. There seem to be some wheels listed to download for all of these projects: https://pypi.org/project/netCDF4/#files |
good to know, thanks. since this PR will really change the developer experience I'll just open RFC PRs for both pixi and uv and see which people prefer. Probably won't get to it until over the weekend though |
Thank you @maxrjones ! Really appreciate your efforts here. |
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.
LGTM! I'll let @TomNicholas provide official approval.
'universal_pathlib @ git+https://github.com/fsspec/universal_pathlib', | ||
'numcodecs @ git+https://github.com/zarr-developers/numcodecs', | ||
'ujson @ git+https://github.com/ultrajson/ultrajson', | ||
'zarr @ git+https://github.com/zarr-developers/zarr-python', |
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.
Should zarr-python from github be an upstream dependency? I know it's also in default dependencies but just curious because zarr-python also a dependency of icechunk.
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.
Since we're using Zarr's internal metadata classes in the develop
branch, I think it's important to explicitly test for compatibility with the latest, unreleased changes in Zarr-Python.
Co-authored-by: Aimee Barciauskas <[email protected]>
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 looks great, I just have a couple of questions so I can understand the choices better.
ci/min-deps.yml
Outdated
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.
What defines the min-deps
dependencies now? The dependencies in the pyproject.toml
?
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.
yeah it's in pyproject.toml
and defined is the combination of these dependency sets:
Line 171 in efcb153
min-deps = ["dev", "hdf", "hdf5", "hdf5-lib"] # VirtualiZarr/conftest.py using h5py, so the minimum set of dependencies for testing still includes hdf libs |
It's unfortunate the s3 (included as part of dev
and the hdf libraries are required right now for running any tests (this was already the case) and would be good to refactor the conftest.py
modules so this isn't the case.
I also added a line to the contributing guide showing how to list the installed environments.
# Run all tests | ||
pixi run --environment test run-tests | ||
# Skip tests that require a network connection | ||
pixi run --environment test run-tests-no-network |
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.
Why is it necessary to make running the tests a pixi
thing, rather than just installing dependencies using pixi and then running tests using pytest
directly?
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 is a shorthand for running a command in a specified shell. I added a note on how to explicitly start a shell to run pytest
in 86ff959
(#407)
pixi install --environment docs | ||
pixi run build-docs | ||
``` | ||
Pixi can also be used to serve continuously updating version of the documentation during development at [http://0.0.0.0:8000/](http://0.0.0.0:8000/). |
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.
Oh that's cool. Will that work with #486?
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.
yeah, unfortunately they'll have some merge conflicts to resolve but it uses the same sphinx-autobuild
extension they reference.
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.
Amazing @maxrjones thank you so much!
Moves from using conda environment files to using pixi to store all dependency information in pyproject.toml
Check out the updated contributing guide to get started.
TODO:
Closes #310