Skip to content

Only adding zarrRootPath field to datasets that use Zarr #137

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

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

Andrewq11
Copy link
Contributor

Changelogs

Updating the client.upload_dataset method to only add the zarrRootPath field to datasets which make use of Zarr. Otherwise, datasets which do not use make use of Zarr will fail a validation check and cannot be uploaded.


Checklist:

- [ ] Was this PR discussed in an issue? It is recommended to first discuss a new feature into a GitHub issue before opening a PR.
- [ ] Add tests to cover the fixed bug(s) or the newly introduced feature(s) (if appropriate).

  • Update the API documentation if a new function is added, or an existing one is deleted.
  • Write concise and explanatory changelogs above.
  • If possible, assign one of the following labels to the PR: feature, fix, chore, documentation or test (or ask a maintainer to do it for you).

discussion related to that PR

@Andrewq11 Andrewq11 added the fix Annotates any PR that fixes bugs label Jul 17, 2024
@Andrewq11 Andrewq11 requested review from jstlaurent and kirahowe July 17, 2024 20:20
@Andrewq11 Andrewq11 self-assigned this Jul 17, 2024
@Andrewq11 Andrewq11 requested a review from cwognum as a code owner July 17, 2024 20:20
@cwognum
Copy link
Collaborator

cwognum commented Jul 17, 2024

I'm okay with merging this to unblock everyone, but this piece of code feels brittle to me (my bad!).

Maybe we should instead improve the check on the Hub side to use the usesZarr field? This is already part of the payload anyways. And maybe the Hub should actually create and return a zarrRootPath to upload to? Something to think about!

@Andrewq11
Copy link
Contributor Author

I'll merge this and create a ticket to improve how we handle validation on the Hub side when it comes to Zarr datasets.

@Andrewq11 Andrewq11 merged commit b893f8e into main Jul 18, 2024
4 checks passed
@jstlaurent jstlaurent deleted the fix/zarr-root-path branch December 4, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Annotates any PR that fixes bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants