Skip to content
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

Merged
merged 2 commits into from
Aug 24, 2021
Merged

Conversation

joshmoore
Copy link
Member

@joshmoore joshmoore commented Aug 24, 2021

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:

channels:
  - conda-forge
  - defaults
dependencies:
  - python < 3.9.0
  - wheel
  - numcodecs
  - numpy
  - pip
  - pip:
    - asciitree
    - fasteners
    - pytest
    - setuptools_scm

@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #819 (c6d1a97) into master (adc430a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #819   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          31       31           
  Lines       10613    10614    +1     
=======================================
+ Hits        10598    10599    +1     
  Misses         15       15           
Impacted Files Coverage Δ
zarr/tests/test_dim_separator.py 97.61% <100.00%> (+0.05%) ⬆️

@joshmoore
Copy link
Member Author

Still ignoring codecov (opinions welcome) and moving forward with this to unblock conda.

@joshmoore joshmoore merged commit 8d699c3 into zarr-developers:master Aug 24, 2021
@joshmoore joshmoore deleted the 2_9_1 branch August 24, 2021 10:18
Copy link
Contributor

@benjaminhwilliams benjaminhwilliams left a 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))
Copy link
Contributor

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:

Suggested change
verify(zarr.open(dataset))
verify(zarr.open(dataset, "r"))

Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

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.

2 participants