Fixed race condition that may result in errors in cleanup and deprecate cleanup

[schlunma]

Description

This PR removes the race condition that may lead to errors in the cleanup preprocessor by using temporary directories to store fixed files.

Closes #1948

Deprecation

This PR deprecates the cleanup preprocessor, which is not needed anymore. Users should not use that anymore.

---

Before you get started

• ☝ Create an issue to discuss what you are going to do

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.

• 🧪 The new functionality is relevant and scientifically sound
• 🛠 This pull request has a descriptive title and labels
• 🛠 Code is written according to the code quality guidelines
• 🧪 and 🛠 Documentation is available
• 🛠 Unit tests have been added
• 🛠 Changes are backward compatible
• 🛠 Any changed dependencies have been added or removed correctly
• 🛠 The list of authors is up to date
• 🛠 All checks below this pull request were successful

---

To help with the number pull requests:

• 🙏 We kindly ask you to review two other open pull requests in this repository

[codecov]

Codecov Report

Merging #1949 (50dfc8e) into main (b1e5fec) will increase coverage by 0.01%.
The diff coverage is 96.36%.

@@            Coverage Diff             @@
##             main    #1949      +/-   ##
==========================================
+ Coverage   92.65%   92.66%   +0.01%     
==========================================
  Files         236      236              
  Lines       12417    12437      +20     
==========================================
+ Hits        11505    11525      +20     
  Misses        912      912              

Impacted Files	Coverage Δ
esmvalcore/_recipe/recipe.py	99.02% <ø> (-0.01%)	⬇️
esmvalcore/config/_config_validators.py	96.93% <ø> (ø)
esmvalcore/cmor/_fixes/ipslcm/ipsl_cm6.py	36.95% <33.33%> (ø)
esmvalcore/_main.py	90.79% <100.00%> (+0.11%)	⬆️
esmvalcore/cmor/_fixes/cmip6/cesm2.py	93.10% <100.00%> (ø)
esmvalcore/cmor/_fixes/cmip6/cesm2_waccm.py	100.00% <100.00%> (ø)
esmvalcore/cmor/_fixes/emac/emac.py	100.00% <100.00%> (ø)
esmvalcore/cmor/_fixes/fix.py	98.59% <100.00%> (+1.66%)	⬆️
esmvalcore/cmor/fix.py	100.00% <100.00%> (ø)
esmvalcore/config/_config_object.py	87.58% <100.00%> (+0.35%)	⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[bouweandela]

Hi @schlunma, I think we may need something that generates a random directory name every time you call Dataset.load, else the issue will still exist when the dataset object is not used from a recipe or if the dataset is loaded twice, eg first to read the vertical levels from it and later to process it.

How about creating a preproc/fixed_files directory and every time Dataset.load is called, create a subdirectory in it using https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp and store the fixed files for that load call in there? This ensures it is always a fresh directory. We can then clean up with a single command after running the entire recipe by simply removing the directory preproc/fixed_files.

[schlunma]

Makes sense. I guess this just hasn't surfaced yet since not many datasets actually use fix_file.

Just to be sure I understand this correctly: create this temporary file for each Dataset, also the non-supplementaries?

[bouweandela]

Just to be sure I understand this correctly: create this temporary file for each Dataset, also the non-supplementaries?

Yes, but a directory instead of a file, since each dataset can consist of multiple files.

[schlunma]

Done!

[valeriupredoi]

approving this but I'd be happier if @bouweandela as the righteous honorable reviewer had a last look. Thanks, gents! One quick q: you making sure those tempdirs get removed right? 🍺

[schlunma]

Did you look at the code before approving? 😝

ESMValCore/esmvalcore/_main.py

Lines 459 to 470 in 2fcdf1f

	if (not session['save_intermediary_cubes'] and
	session.fixed_file_dir.exists()):
	logger.debug(
	"Removing `preproc/fixed_files` directory containing fixed "
	"data"
	)
	logger.debug(
	"If this data is further needed, then set "
	"`save_intermediary_cubes` to `true` and `remove_preproc_dir` "
	"to `false` in your user configuration file"
	)
	shutil.rmtree(session.fixed_file_dir)

[valeriupredoi]

Did you look at the code before approving? stuck_out_tongue_closed_eyes

ESMValCore/esmvalcore/_main.py

Lines 459 to 470 in 2fcdf1f

	if (not session['save_intermediary_cubes'] and
	session.fixed_file_dir.exists()):
	logger.debug(
	"Removing `preproc/fixed_files` directory containing fixed "
	"data"
	)
	logger.debug(
	"If this data is further needed, then set "
	"`save_intermediary_cubes` to `true` and `remove_preproc_dir` "
	"to `false` in your user configuration file"
	)
	shutil.rmtree(session.fixed_file_dir)

shutil.rmtree(Manu) pshhh 😆 Yeah I missed a big chunk indeed 😁

[bouweandela]

Since the cleanup preprocessor function is no longer usable since #1609, we might as well remove it completely. Is that something that we would like to do in this pull request or in a different pull request?

[schlunma]

Removing cleanup would introduce yet another deprecation, but let me see what I can do.

[schlunma]

(I won't address the missing test in the IPSL CMORizer)

[bouweandela]

Sorry to come up with this suggestion so late, but thinking about this a bit more: we're now creating a huge number of output directories for fix_file (one for every dataset load), while very few datasets need this. It would be better only to create the directory when we actually need it. Shall we move the temporary directory creation to the function Fix.get_fixed_filepath instead? I added some code suggestions below to do this.

[remi-kazeroni]

Removing cleanup would introduce yet another deprecation, but let me see what I can do.

Thanks a lot for your work @schlunma! Please remember to use the "deprecated feature" label if that's needed and add a note about the deprecation in the PR header 🍻

[schlunma]

Sorry to come up with this suggestion so late, but thinking about this a bit more: we're now creating a huge number of output directories for fix_file (one for every dataset load), while very few datasets need this. It would be better only to create the directory when we actually need it. Shall we move the temporary directory creation to the function Fix.get_fixed_filepath instead? I added some code suggestions below to do this.

This used to be the behavior pre-#1609, but has been changed here:

ESMValCore/esmvalcore/cmor/fix.py

Lines 58 to 59 in f87e25e

	if not output_dir.exists():
	output_dir.mkdir(parents=True, exist_ok=True)

I thought this was on purpose, that's why I did not change that here...I will change it.

[bouweandela]

Great, thanks!

[schlunma]

Done. The fixed file directory is now only created if necessary. Note that I added an optional argument to fix_file to ensure backwards-compatibility.

[bouweandela]

Thanks for fixing this @schlunma!

I made a small final change myself in 50dfc8e instead of putting in another review comment, to not hold up the release process any longer. I hope that's OK with you.

[remi-kazeroni]

Thanks very much @schlunma for tackling this last minute issue before the feature freeze! And also many thanks to @bouweandela and @valeriupredoi for the reviews! I'm going to merge now. This means: feature freeze is now and we're finalizing v2.8.0rc1 🍺