Skip to content

(chore) use pixi for building #704

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Feb 9, 2025

Adds pixi for building the project. On my linux desktop, everything compiles with
pixi run pip install -v -e '.[test,test_extras,msgpack,crc32c,pcodec,zfpy]'.

@d-v-b d-v-b marked this pull request as draft February 9, 2025 11:15
@d-v-b
Copy link
Contributor Author

d-v-b commented Feb 9, 2025

not sure if adding the blosc dep here is correct, since we are vendoring it (cc @jakirkham)

Copy link

codecov bot commented Feb 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.92%. Comparing base (506c89b) to head (4dfd79d).

❌ Your project check has failed because the head coverage (90.92%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (506c89b) and HEAD (4dfd79d). Click for more details.

HEAD has 10 uploads less than BASE
Flag BASE (506c89b) HEAD (4dfd79d)
15 5
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #704      +/-   ##
==========================================
- Coverage   99.96%   90.92%   -9.04%     
==========================================
  Files          64       64              
  Lines        2789     2789              
==========================================
- Hits         2788     2536     -252     
- Misses          1      253     +252     

see 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@d-v-b
Copy link
Contributor Author

d-v-b commented Feb 9, 2025

cc @mkitti in case you have any advice

@mkitti
Copy link
Contributor

mkitti commented Feb 14, 2025

not sure if adding the blosc dep here is correct, since we are vendoring it (cc @jakirkham)

I'll just point out that blosc is not even a dependency of the numcodecs package in conda-forge:
https://github.com/conda-forge/numcodecs-feedstock/blob/main/recipe/meta.yaml

I think this situation is a very weird. We're not even modifying the vendored blosc build here.

In contrast, the blosc package in conda-forge depends on lz4-c, snappy, zlib, and zstd packages, as this package should.
https://github.com/conda-forge/blosc-feedstock/blob/main/recipe/meta.yaml

That said, I have no idea how one would have those dependencies in PyPI via wheels.

@martindurant
Copy link
Member

martindurant commented Feb 14, 2025

cramjam, anyone?? This would just be the reason for them to solidify the blosc backend, but they have all the others in a statically-linked wheel

@mkitti
Copy link
Contributor

mkitti commented Feb 14, 2025

statically-linked wheel

Is a big statically-linked wheel the ideal here?

@martindurant
Copy link
Member

martindurant commented Feb 14, 2025

Is a big statically-linked wheel the ideal here?

If you're worried about dependencies, packaging and how to link to system libs, I suggest that yes. I note that cramjam is ~2MB. Also, zarr likes rust crates, and cramjam can be called directly from rust, and also return python-consumable buffers.

@mkitti
Copy link
Contributor

mkitti commented Feb 14, 2025

I took a look at libcramjam. I can already see several major issues. For example, I do not see checksum options for Zstandard. Now in certain circumstances, perhaps this could be fine if we had a plugin system that allowed for multiple implementations of the same codec.

I think I would much rather have a direct dependency on libzstd rather than going through another intermediary.

@mkitti
Copy link
Contributor

mkitti commented Jun 28, 2025

Rather than

pixi run pip install -v -e '.[test,test_extras,msgpack,crc32c,pcodec,zfpy]'

I think either pixi install or pixi reinstall should be sufficient. Pixi should be able to handle all the dependencies. If the optional dependencies are desired, they could be inserted via features and (sub)environments.

pixi reinstall is relatively recent.

@d-v-b d-v-b mentioned this pull request Jul 19, 2025
7 tasks
@cas--
Copy link

cas-- commented Jul 21, 2025

I noticed the config has an editable install, just be aware there are two open Pixi issues about editable installs causing lockfile churn:

@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 21, 2025

the editable install is not at all required, feel free to amend that if it makes things easier. i'm far from a pixi expert so I'm happy to defer to others here!

@mkitti
Copy link
Contributor

mkitti commented Jul 21, 2025

I noticed the config has an editable install, just be aware there are two open Pixi issues about editable installs causing lockfile churn.

Why is this a problem? For a library like this, we could just add the lock file to .gitignore.

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