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

Updated doc about fixes and added type hints to fix functions #2160

Merged
merged 14 commits into from
Aug 9, 2023

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Aug 1, 2023

Description

This PR updates the outdated doc about fixes (add the necessary function get_cube_from_list to fix_metadata and replaced the old extract_strict with extract_cube) and adds type hints to many fix-related functions.

There is NO change in functionality.

Links to 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 pull requests:

@schlunma schlunma added the fix for dataset Related to dataset-specific fix files label Aug 1, 2023
@schlunma schlunma added this to the v2.10.0 milestone Aug 1, 2023
@schlunma schlunma self-assigned this Aug 1, 2023
@schlunma schlunma changed the title Allow fix_file/load to return/process CubeList Allow fix_file/load to return/read CubeList objects Aug 1, 2023
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #2160 (81d43ce) into main (facd794) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2160   +/-   ##
=======================================
  Coverage   93.10%   93.10%           
=======================================
  Files         237      237           
  Lines       12816    12822    +6     
=======================================
+ Hits        11932    11938    +6     
  Misses        884      884           
Files Changed Coverage Δ
esmvalcore/cmor/fix.py 100.00% <ø> (ø)
esmvalcore/cmor/_fixes/fix.py 98.59% <100.00%> (+0.02%) ⬆️
esmvalcore/preprocessor/_io.py 87.10% <100.00%> (+0.25%) ⬆️

@schlunma schlunma mentioned this pull request Aug 1, 2023
@schlunma schlunma changed the title Allow fix_file/load to return/read CubeList objects Updated doc about fixes and added type hints to fix functions Aug 2, 2023
Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

thanks, Manu! A couple minor points from me 🍺

doc/conf.py Show resolved Hide resolved
esmvalcore/cmor/_fixes/fix.py Outdated Show resolved Hide resolved
esmvalcore/preprocessor/_io.py Outdated Show resolved Hide resolved
esmvalcore/preprocessor/_io.py Outdated Show resolved Hide resolved
for cube in raw_cubes:
cube.attributes['source_file'] = file
cube.attributes['source_file'] = str(file)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would put a check on file's type in here (str is a bit too general, so we are exposing us to fails if load is called with load("s3://cow")) - a placeholder so we maybe get support for eg object storage "files" in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry - I was a bit too vague on this: I'd defo investigate what type of str is supported inside the load func (or maybe create an IO or Load error class); also maybe nice to add a test case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't fully get your comment - at the moment, only valid Path and str objects can be passed to load. Since a couple of version iris can digest Path objects, that's why I am calling Path(file) at the beginning of the function now. Here, we needs str(file) for the attribute to be properly set.

Since now other types are supported, I don't think we need to add additional checks here. If the path is invalid, we will either get an iris error during iris.load_raw or a ValueError from our code.

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, and my second phrasing of the comment was as vague as a Tory declaring their taxes - what I really meant was we should probably think of examining the str path a bit more in the context when it is not a valid string of a POSIX path, and throw up the error, but a little bit more tailored towards cases such eg the str path starts with "s3" or "https" - the first steps towards supporting remote/object store files. Clearly, not a priority now, so totally up to you if you want to go start going down this rabbit hole 🐰

schlunma and others added 3 commits August 3, 2023 08:30
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
@valeriupredoi
Copy link
Contributor

thanks a lot for addressing the review comments @schlunma - if you don't (and I expect you not to) want do anything about #2160 (comment) then this is good to be merged, you wanna poke the TLT for a merge? 🍺

@schlunma
Copy link
Contributor Author

schlunma commented Aug 3, 2023

Thanks for reviewing V! You're right, I don't want to address this here 😅

@schlunma
Copy link
Contributor Author

schlunma commented Aug 8, 2023

In order to proceed with #2157, it would be nice to have this merged today (there will be a lot of merge conflicts which I would like to solve before finalizing #2157). Since this is a pure maitenance PR, I don't think this be a problem. Any objections @ESMValGroup/technical-lead-development-team?

@remi-kazeroni remi-kazeroni merged commit 6b43ca2 into main Aug 9, 2023
@remi-kazeroni remi-kazeroni deleted the rethink_fix_file branch August 9, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix for dataset Related to dataset-specific fix files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants