-
Notifications
You must be signed in to change notification settings - Fork 146
Update the CMORizer command #4317
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
Conversation
41f3f9e to
ca8b258
Compare
|
The two remaining Codacy issues are about imports that only work for yet to be released ESMValCore v2.14, so these don't seem easy to fix. |
|
On it in a wee bit, bud, cheers 🍺 |
valeriupredoi
left a comment
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 very good and it looks like a lot of work since you've also improved bits that were a bit long in the tooth - very many thanks @bouweandela 🍻 Question is - do you want to wait until the release for merging? All the tests that are run with the esmvalcore package from conda-forge will fail if we merge now
| "Please specify the correct path to the input data using the " | ||
| "--original-data-dir flag." | ||
| ) | ||
| raise NotADirectoryError(msg) |
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.
I reckon we should start thinking object stores even for this rather low level operation (ie cmorization), so this should not be a problem in the future, when we'd have URLs as "original directory"
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.
Yes, but maybe not with this collection of scripts. As part of the ESO4clima project, we'll be developing an in-memory CMORizer package that works directly with Xarray datasets as they are loaded by xcube from ESA Climate Data Centre, similar to how we CMORize ERA5 in ESMValCore but a standalone package that will be integrated with ESMValCore. I'll try to make it generic enough so it can also be configured to work with other datasets.
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.
ooh the future IS here then - hurry up and merge this, I'd like to finish me lunch, not get intrigued by your comments (intrigued in a positive manner) 😁
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.
lunch finished long time ago, am hungry again though, and still no merge - on a serious note, I reckon it's well ready, when you come back tomorrow 🍺
As far as I'm aware, they are all passing (see the CircleCI tests runs) below. The only backwards incompatible change is that you can't call the |
and we don't have tests that run that command? Nevermind then 😁 |
|
Test coverage is pretty poor indeed, I've mostly tested it by running a Python and an NCL CMORizer script manually. |
shh 🤫 |
rootpath:
RAWOBS: /work/bd0854/DATA/ESMValTool2/RAWOBSseems to ignore the specified path: $ esmvaltool data prepare Yang2020
/home/b/b309141/repos/ESMValCore/esmvalcore/config/_config_validators.py:521: ESMValCoreDeprecationWarning: The configuration option or command line argument `rootpath` has been deprecated in ESMValCore version 2.14.0 and is scheduled for removal in version 2.16.0. Please configure data sources under `projects` ins
tead.
_handle_deprecation(option, deprecated_version, remove_version, more_info)
[...]
2026-02-02 11:42:25,060 UTC [1947588] INFO input_dir = /home/b/b309141
[...]Please read our policy on backwards-incompatible changes:
This has been merged after less than two weeks. I guess the preferred option would be to deprecate this properly and remove it in two versions like we usually do (although I am surprised that this is not described in our policy on backwards-incompatible changes).
AttributeError: 'str' object has no attribute 'is_dir'It looks like this has not been tested once before merging this. |
|
Thanks for raising these concerns @schlunma, the backwards incompatible changes were not intentional (except for the ones described in the top post, for which the proper procedure was followed). I did ask on the tech lead team Slack for a second reviewer, but after not getting a response from anyone for a week I thought it was time to move forward. Anyway, I'll try to fix the issues you found in a new pull request. |
Manu, you shouldn't expect testing all the cmorizers - if there are no integration tests to do that, then it's gonna have to be on a fix as we go basis, when a fault is discovered 🍺 |
I certainly don't expect anyone to run all CMORizers (I am fairly certain that some of them, especially the older ones, might not work due to countless updates of underlying code), but I think it would have been nice to run the command in question once (with a random data set) to catch any obvious problems. |
we need more revieweing hands, as Bouwe mentioned, I only went over the code myself 🍺 |
Description
--config-fileargument.--original-data-dirargument to replace the use ofesmvalcore.config.CFG["rootpath"]["RAWOBS"]which is no longer used by ESMValCore since Add an interface for adding new data sources and add support for intake-esgf as a first example ESMValCore#2765.Backwards incompatible changes
The
--config-fileargument to theesmvaltool datacommands has been removed. Upgrade instructions: please use these commands with the--config-dirargument to specify the output directory, custom CMOR tables, or Dask configuration, and with the--original-data-dirargument to point the tool to the directory where downloaded original data should be saved.Closes #4313
Links to changed documentation:
Before you get started
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.
To help with the number of pull requests: