-
-
Notifications
You must be signed in to change notification settings - Fork 314
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
Zarr 2.9.1: fix conda release #819
Conversation
Codecov Report
@@ Coverage Diff @@
## master #819 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 31 31
Lines 10613 10614 +1
=======================================
+ Hits 10598 10599 +1
Misses 15 15
|
Still ignoring codecov (opinions welcome) and moving forward with this to unblock conda. |
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'm not sure whether you welcome reviews from outsiders to the project, but I think there's slightly more to the Conda release failure than just catching absent fsspec
. See my suggestion.
@@ -63,6 +63,7 @@ def test_open(dataset): | |||
verify(zarr.open(dataset)) |
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 think there's an additional problem with test_open[static_flat]
and test_open[static_nested]
, which is also blocking the Conda release. The Pytest working directory does not contain the expected paths fixture/flat
and fixture/nested
, which becomes clearer if you're stricter about the persistence mode you use in test_open
:
verify(zarr.open(dataset)) | |
verify(zarr.open(dataset, "r")) |
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 has the added benefit of guarding against accidentally leaving any little droppings in the Pytest working directory.
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.
Hi @benjaminhwilliams, I at least welcome it! ;) Would you be up for opening this as a PR?
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.
Sure. This small suggestion only highlights the issue, it doesn't fix the problem that the Pytest working directory is not the project root, as the test appears to assume. But we can fix that by discussion in the PR. I'll open it and request your review.
see:
The conda-forge release failed since fsspec is not installed by default. In order to get Zarr deployed ASAP (re: DiamondLightSource/python-tristan#62) this simply properly handles the test issue that @QuLogic brought up in #817.
It might be that moving forward, fsspec should be installed & tested for conda-forge releases. If not, then a minimal test workflow should be added here to show that the conda-forge requirements (below) suffice to run or skip all tests: