-
Notifications
You must be signed in to change notification settings - Fork 42
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
xarray version update #323
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks great! Thanks for the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a concern.
@@ -31,5 +30,6 @@ dependencies: | |||
- pygrib=2.1.4 | |||
- pip: | |||
- earthengine-api==0.1.329 | |||
- xarray==2023.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second though: we may get weird link errors with our conda deploy if we don't include this as a conda dependency. Has this been published on conda forge yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While trying to install the xarray=2023.1.0
package in the CI/CD pipeline via the conda-forge
channel, I encountered an error. However, the installation process worked without any issues when I tried it locally. Here's an error message that I received during the CI/CD run:
Link
Do you have any suggestions for resolving this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like 2023.4.2 is available on conda (I think this is the latest release). How about we try upgrading to that version?
https://github.com/conda-forge/xarray-feedstock#current-release-info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the PR Description This is the last updated version of xarray that python 3.8 supported
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm getting ahead of myself. I'm happy to keep the pip (vs the conda) version, if we can test our local and docker deployment to prove that there are no link errors. Let's also create a follow-up CL to deprecate python 3.8 and include python 3.10. How does this sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @alxmrs, I have tested both the local
and docker
deployments and did not encounter any errors.
Yes, this sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Solution for the TODO of issue #291
TODO: Find a better way for logging the dataset size.
is just updating the version of xarray.The problem with the current version of xarray which we are using is it takes too much memory while calculating
dataset.nbytes
(eg:Line #408 weather-mv
) and kills the process as well.To address the problem, I reported the issue on the official xarray GitHub page pydata/xarray#7772 and they suggested to use the latest version of xarray. After testing, I found that xarray
v2023.1.0
worked well, and we no longer experienced any memory issues.ps: This is the last
python 3.8
supported version of xarray.