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

196 attempt to detect filename when saving #218

Merged
merged 38 commits into from
Feb 14, 2024

Conversation

EddyCMWF
Copy link
Contributor

What do you think of this? Maybe one to discuss in an earthkit-data meeting?
The issue #196 has the back story

@EddyCMWF EddyCMWF linked an issue Oct 12, 2023 that may be closed by this pull request
@sandorkertesz
Copy link
Collaborator

Please can you add a comment showing some examples with explanation? #196 does not contain these.

@EddyCMWF
Copy link
Contributor Author

EddyCMWF commented Oct 17, 2023

Please can you add a comment showing some examples with explanation? #196 does not contain these.

Here is the example I am working towards:

s = from_source(
    "cds",
    "reanalysis-era5-single-levels",
    variable=["2t"],  #, "msl"],
    product_type="reanalysis",
    area=[50, -50, 20, 50],
    date="2007-01-01/2008-02-04",
    time="12:00",
)
s.save()

I will add to the cds notebook, and something similar in a URL notebook.

Working on this has raised a good point, it looks like we have already thrown away the real filename by the time we get to the decorator, so the filename used is the just the earthkit cache hash. I will also look into how we can handle this better.

@EddyCMWF EddyCMWF closed this Oct 18, 2023
@EddyCMWF
Copy link
Contributor Author

Hi @sandorkertesz ,
I've reopened this PR, it now includes safety features which prevent users from over-writing the file they are reading, and we preserve the source_filename from the CDS.

I'm still not convinced by the behaviour when the result is an archive, but there are other deeper issues I have there, so I will create a new issue for that.

Eddy

@EddyCMWF
Copy link
Contributor Author

EddyCMWF commented Feb 2, 2024

Hi @sandorkertesz ,
Is there anything blocking this being merged?
Eddy

@sandorkertesz
Copy link
Collaborator

Hi @sandorkertesz , Is there anything blocking this being merged? Eddy

There are some pending comments. Otherwise, I just need to check it again, it has been a long time since I last looked into it

@EddyCMWF
Copy link
Contributor Author

EddyCMWF commented Feb 7, 2024

Hi @sandorkertesz , I don't see any unresolved comments, could you point me to them?

@sandorkertesz
Copy link
Collaborator

sandorkertesz commented Feb 7, 2024

Hi @sandorkertesz , I don't see any unresolved comments, could you point me to them? Can you see any of them?

I am sorry, I did not submit them, so you could not see them!

earthkit/data/core/fieldlist.py Show resolved Hide resolved
earthkit/data/readers/grib/reader.py Outdated Show resolved Hide resolved
earthkit/data/readers/__init__.py Outdated Show resolved Hide resolved
earthkit/data/sources/cds.py Show resolved Hide resolved
docs/examples/temp.nc Outdated Show resolved Hide resolved
tests/sources/test_cds.py Outdated Show resolved Hide resolved
@EddyCMWF
Copy link
Contributor Author

Hi @sandorkertesz , tests are passing so hopefully good to go.
Thanks for the help!
Eddy

@sandorkertesz
Copy link
Collaborator

Hi @EddyCMWF, thanks for the changes, it is now in a good shape. Please squash and merge this PR.

@EddyCMWF
Copy link
Contributor Author

Hi @EddyCMWF, thanks for the changes, it is now in a good shape. Please squash and merge this PR.

Thanks Sandor, but I don't have permissions to do the squash and merge

@sandorkertesz sandorkertesz merged commit c6266eb into develop Feb 14, 2024
57 checks passed
@sandorkertesz
Copy link
Collaborator

Hi @EddyCMWF, merged and the branch can be deleted.

@EddyCMWF EddyCMWF deleted the 196-attempt-to-detect-filename-when-saving branch February 14, 2024 15:09
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.

Attempt to detect filename when saving
2 participants