Skip to content

Fix running unit tests locally #68

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

samueljackson92
Copy link
Collaborator

There seems to be some issue with running the unit tests with the zarr version.

This fixes that issue, and as a bonus enables pyuda tests when the user is connected to the UKAEA VPN.

To test:

  • Run uv run pytest
  • Code review!

@samueljackson92 samueljackson92 added the enhancement New feature or request label Apr 9, 2025
@samueljackson92 samueljackson92 self-assigned this Apr 9, 2025
Copy link
Collaborator

@jameshod5 jameshod5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine to me, just wanted to ask about the docs on the zarr changes before approving

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix I was using, but in the docs: https://docs.xarray.dev/en/stable/generated/xarray.Dataset.to_zarr.html

It states that zarr_version is being depreciated? I'm not sure if this is a wider zarr bug

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swapped back to zarr_version for now.

@jameshod5
Copy link
Collaborator

Raised issue with xarray: link

@samueljackson92
Copy link
Collaborator Author

All fixed now I think...

@NathanCummings
Copy link
Member

  1. The zarr_version vs zarr_format issue may be solved with a bump to Xarray version from what I gather, but that can be a separate PR - we can cope with a few deprecation warnings.
  2. try_uda() makes me feel the bad feelings, but I guess it is fine...

Happy for this to be merged if everyone else is.

Copy link
Collaborator

@jameshod5 jameshod5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

It seems we need to upgrade xarray to 2024.10.0 in order for us to use zarr_format. Is it worth doing that now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants