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

Avoid importing zarr library unless necessary #17

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Nov 14, 2023

At the moment, importing zarr_checksum automatically & unconditionally causes zarr to be imported as well, which results in numpy being imported, which is not good for dandi-cli's "no heavy imports" test. This PR moves the single import of zarr to inside the only function that makes use of it.

@jwodder jwodder requested a review from jjnesbitt November 14, 2023 16:55
Run poetry version patch
@yarikoptic yarikoptic added the internal Changes only affect the internal API label Nov 16, 2023
@yarikoptic
Copy link
Member

Since I guess we would need a released version for dandi-cli, marking it for a release.

@yarikoptic
Copy link
Member

oh, this is not automated and not maintained by "us", thus I guess I should not click "merge". Who is to click it and to cut a release @AlmightyYakob @waxlamp ??

@jwodder
Copy link
Member Author

jwodder commented Nov 16, 2023

@yarikoptic My understanding is that all PRs in this repo get a release automatically unless the "skip-release" label is present. This repo doesn't use auto, so I don't think the "internal" label does anything for releases.

@yarikoptic
Copy link
Member

you are right -- I think it is this workflow https://github.com/dandi/zarr_checksum/blob/master/.github/workflows/python-publish.yml which does it. Cool. So we just need a merge I guess. Let's wait a tiny bit for an authority to give blessing.

@jjnesbitt jjnesbitt merged commit e215073 into master Nov 16, 2023
9 checks passed
@jjnesbitt jjnesbitt deleted the avoid-zarr-import branch November 16, 2023 21:50
jwodder added a commit to dandi/dandi-cli that referenced this pull request Nov 27, 2023
- We can't update to `zarr_checksum` 0.3.0 yet, as that requires Pydantic v2.

- As `zarr_checksum` gained type annotations in dandi/zarr_checksum#21, we can
  remove its override from the mypy config.

- Since dandi/zarr_checksum#17 was merged, it's now safe to import
  `zarr_checksum` at the top level of a module.

- Fixed a typing error caused by importing `ZarrChecksumTree` from the top
  level of `zarr_checksum` despite it not being listed in
  `zarr_checksum.__all__`, which triggered a typing failure due to our mypy
  configuration containing `implicit_reexport = False`.  (This error could only
  be detected after `zarr_checksum` started shipping a `py.typed` file.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants