Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork366
Fix hatch matrix setup for minimal and optional dependencies#2872
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
sounds good to me |
Tests are failing because the environment name changed, I'll fix this in a bit |
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.
Very 👍 this
Uh oh!
There was an error while loading.Please reload this page.
| [[tool.hatch.envs.test.matrix]] | ||
| python = ["3.11","3.12","3.13"] | ||
| numpy = ["1.25","2.1"] | ||
| version = ["minimal"] | ||
| [[tool.hatch.envs.test.matrix]] | ||
| python = ["3.11","3.12","3.13"] | ||
| numpy = ["1.25","2.1"] | ||
| features = ["optional"] | ||
| deps = ["minimal","optional"] | ||
| [[tool.hatch.envs.test.matrix]] | ||
| python = ["3.11","3.12","3.13"] | ||
| numpy = ["1.25","2.1"] | ||
| features = ["gpu"] | ||
| [tool.hatch.envs.test.overrides] | ||
| matrix.deps.dependencies = [ | ||
| {value ="zarr[remote, remote_tests, test, optional]",if = ["optional"]} | ||
| ] |
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.
I determined by looking at the old logs that there were no actual differences between the minimal and optional groups of the matrix. This override setup properly includes optional dependencies for the optional group and excludes them from the minimal group.
| [[tool.hatch.envs.test.matrix]] | ||
| python = ["3.11","3.12","3.13"] | ||
| numpy = ["1.25","2.1"] | ||
| features = ["gpu"] |
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 separately have thetestgpu environment, so there's no need to include another gpu feature set in the regulartest matrix. I removed it because it only makes the setup more confusing.
| run:| | ||
| hatch env run --env test.py${{ matrix.python-version }}-${{ matrix.numpy-version }}-${{ matrix.dependency-set }} run-coverage | ||
| -name:Upload coverage | ||
| if:${{ matrix.dependency-set == 'optional' && matrix.os == 'ubuntu-latest' }} |
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 minimal environment will have less coverage than optional, so there's no value in uploading it to codecov
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.
Is there a downside to uploading it? I think it might be worth just keeping all the uploads to keep the complexity of this file down.
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 seemed to me a tradeoff between complexity in the workflow and complexity in understanding the codecov logs -https://app.codecov.io/gh/zarr-developers/zarr-python/commit/ebadec01e74acdd5bc3273be771c20a0f6a67cce. But not many people will interact with those logs so I can remove this line
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.
That's fair - I tend not to look at the logs, but agree that it makes the logs a bit easier. Lets leave the line in and I'll merge 🚢
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.
For simplicity, I think might be worth keeping the codecov upload? Let me know what you think either way - otherwise this is looking 👍
| run:| | ||
| hatch env run --env test.py${{ matrix.python-version }}-${{ matrix.numpy-version }}-${{ matrix.dependency-set }} run-coverage | ||
| -name:Upload coverage | ||
| if:${{ matrix.dependency-set == 'optional' && matrix.os == 'ubuntu-latest' }} |
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.
Is there a downside to uploading it? I think it might be worth just keeping all the uploads to keep the complexity of this file down.
5f49d24 intozarr-developers:mainUh oh!
There was an error while loading.Please reload this page.
…velopers#2872)* Run minimal tests without fsspec, requests, aiohttp* Retain existing test env names* Use importorskip* Specify which matrix config to upload codecov on* Remove redundant gpu env* Add obstore to min_deps definition* Fix optional dependency set* Add remote_tests set to doctest
I noticed after Martin's comment in#2774 (comment) that even the 'minimal' tests include optional dependencies including fsspec and universal_pathlib. I think it's worth only including the required dependencies and those necessary for running tests / type checking in the "minimal" test environment.