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

Unpin netcdf4 and pandas to check what is breaking [all tests ci] #1111

Closed
wants to merge 3 commits into from

Conversation

ocefpaf
Copy link
Contributor

@ocefpaf ocefpaf commented Aug 2, 2023

I'd like to run this locally but I don't have access to the test data drive to run the full test suite.

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Aug 2, 2023

@lsetiawan I need your help understanding the CIs here. It seems that the tests did not run, is that correct?

@lsetiawan lsetiawan changed the title Unpin netcdf4 and pandas to check what is breaking Unpin netcdf4 and pandas to check what is breaking [all tests ci] Aug 2, 2023
@lsetiawan lsetiawan closed this Aug 2, 2023
@lsetiawan lsetiawan reopened this Aug 2, 2023
@emiliom
Copy link
Collaborator

emiliom commented Aug 2, 2023

Thanks for chiming in @ocefpaf ! First, our development branch is dev; I suggest running tests there rather than on main. In dev we already removed the Pandas pinning, and all is good there. But we are running into a number of issues with netCDF4. #988 describes one of them, but lately we've been running into more that we haven't documented in a GitHub issue yet.

The test data are packaged in a Docker image that's rebuilt daily. The tests can be run locally with these instructions, which first deploy two docker images that include the test data.

@lsetiawan
Copy link
Member

The current failure seen in CI is exactly this one: Unidata/netcdf4-python#1175

It is the reason we pinned 😬

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Aug 2, 2023

@lsetiawan I could not find any other issue besides the variable length compression. If so, let me know. I guess you have everything you need to bring this one home. I'll close this b/c you know better the internals of the package to find the strings and other vlen, to remove compression from them.

PS: As we discussed on slack, Unidata/netcdf-c#2716 will "fix this" but it is better to explicitly disable compression on variables that won't be compressed IMO. Also, that would fix this right now instead of whenever they make a new release ;-p

@ocefpaf ocefpaf closed this Aug 2, 2023
@ocefpaf ocefpaf deleted the unpin_netcdf4 branch August 2, 2023 18:03
@ocefpaf
Copy link
Contributor Author

ocefpaf commented Aug 2, 2023

The current failure seen in CI is exactly this one: Unidata/netcdf4-python#1175

This one Unidata/netcdf4-python#1205 probably describes is better and has the links to the issue upstream in netcdf-c.

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Aug 2, 2023

Thanks for chiming in @ocefpaf ! First, our development branch is dev; I suggest running tests there rather than on main. In dev we already removed the Pandas pinning, and all is good there. But we are running into a number of issues with netCDF4. #988 describes one of them, but lately we've been running into more that we haven't documented in a GitHub issue yet.

The test data are packaged in a Docker image that's rebuilt daily. The tests can be run locally with these instructions, which first deploy two docker images that include the test data.

I don't have any stake on this but having non-standard branches, convoluted way to obtain data for running tests, and title coded messages to activate them, makes this less open to contributions. I would suggest to have at least a subset of the tests to run without jumping through hoops. In the long term, have the test data automatically fetched with pooch would be even better.

Anyway, thanks to @lsetiawan's guidance we figured out the problem and a fix is on the way!

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.

3 participants