Skip to content

Deprecate defining write_plots and write_netcdf in config-user file #808

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.

Sign up for GitHub

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

Merged
bouweandela merged 14 commits into from
Jan 29, 2021
Merged

Deprecate defining write_plots and write_netcdf in config-user file #808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9d40f74
Deprecate use of write_plots, write_netcdf, output_file_type from con…
bouweandela Oct 2, 2020
5d9f66c
Add a note in documentation
bouweandela Oct 2, 2020
0c2dccf
Fix indentation
bouweandela Oct 2, 2020
ddd6e17
Use DEPRECATED instead of TODO to mark deprecated stuff
bouweandela Oct 2, 2020
1a9d6dc
Restore default values and add warning about use from cli
bouweandela Oct 5, 2020
822afd5
Increase version in which feature will be removed to v2.4
bouweandela Dec 8, 2020
6a0a73b
Merge branch 'master' into deprecate-write-plots-netcdf-type
bouweandela Jan 5, 2021
a63c460
Merge branch 'master' into deprecate-write-plots-netcdf-type
bouweandela Jan 5, 2021
053e097
Update text to v2.2
bouweandela Jan 5, 2021
0e8dba5
Correct version in which feature will be removed
bouweandela Jan 27, 2021
9ac52c6
Merge branch 'master' of github.com:esmvalgroup/esmvalcore into depre…
bouweandela Jan 27, 2021
29a944c
Merge branch 'master' of github.com:esmvalgroup/esmvalcore into depre…
bouweandela Jan 27, 2021
e9c00f1
Do not deprecate output_file_type just yet
bouweandela Jan 27, 2021
c8c7f9b
Correct version in which feature will be removed
bouweandela Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 7 additions & 20 deletions doc/quickstart/configure.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,6 @@ with explanations in a commented line above each option:

.. code-block:: yaml

# Diagnostics create plots? [true]/false
# turning it off will turn off graphical output from diagnostic
write_plots: true

# Diagnostics write NetCDF files? [true]/false
# turning it off will turn off netCDF output from diagnostic
write_netcdf: true

# Set the console log level debug, [info], warning, error
# for much more information printed to screen set log_level: debug
log_level: info
Expand Down Expand Up @@ -117,19 +109,14 @@ with explanations in a commented line above each option:
drs:
CMIP5: default

Most of these settings are fairly self-explanatory, e.g.:

.. code-block:: yaml

# Diagnostics create plots? [true]/false
write_plots: true
# Diagnostics write NetCDF files? [true]/false
write_netcdf: true
..
DEPRECATED: remove in v2.4

The ``write_plots`` setting is used to inform ESMValTool diagnostics about your
preference for creating figures. Similarly, the ``write_netcdf`` setting is a
boolean which turns on or off the writing of netCDF files by the diagnostic
scripts.
There used to be a setting ``write_plots`` and ``write_netcdf``
in the config user file, but these have been deprecated since ESMValCore v2.2 and
will be removed in v2.4, because only some diagnostic scripts supported these settings.
For those diagnostic scripts that do support these settings, they can now be configured
in the diagnostic script section of the recipe.

.. code-block:: yaml

Expand Down
6 changes: 2 additions & 4 deletions doc/quickstart/output.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ will produce a unique settings.yml file.

The settings.yml file passes several global level keys to diagnostic scripts.
This includes several flags from the config-user.yml file (such as
'write_netcdf', 'write_plots', etc...), several paths which are specific to the
'log_level'), several paths which are specific to the
diagnostic being run (such as 'plot_dir' and 'run_dir') and the location on
disk of the metadata.yml file (described below).

Expand All @@ -97,8 +97,6 @@ disk of the metadata.yml file (described below).
script: Scalar_timeseries
version: 2.0a1
work_dir: [...]recipe_ocean_bgc_20190118_134855/work/diag_timeseries_scalars/Scalar_timeseries
write_netcdf: true
write_plots: true

The first item in the settings file will be a list of `Metadata.yml`_ files.
There is a metadata.yml file generated for each field in each diagnostic.
Expand Down Expand Up @@ -146,5 +144,5 @@ As you can see, this is effectively a dictionary with several items including
data paths, metadata and other information.

There are several tools available in python which are built to read and parse
these files. The tools are avaialbe in the shared directory in the diagnostics
these files. The tools are available in the shared directory in the diagnostics
directory.
28 changes: 26 additions & 2 deletions esmvalcore/_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import datetime
import logging
import os
import warnings
from pathlib import Path

import yaml
Expand Down Expand Up @@ -42,15 +43,35 @@ def read_config_user_file(config_file, folder_name, options=None):
with open(config_file, 'r') as file:
cfg = yaml.safe_load(file)

# DEPRECATED: remove in v2.4
for setting in ('write_plots', 'write_netcdf'):
if setting in cfg:
msg = (
f"Using '{setting}' in {config_file} is deprecated and will "
"be removed in ESMValCore version 2.4. For diagnostics "
"that support this setting, it should be set in the "
"diagnostic script section of the recipe instead. "
f"Remove the setting from {config_file} to get rid of this "
"warning message.")
print(f"Warning: {msg}")
warnings.warn(DeprecationWarning(msg))

if options is None:
options = dict()
for key, value in options.items():
cfg[key] = value
# DEPRECATED: remove in v2.4
if key in ('write_plots', 'write_netcdf'):
msg = (
f"Setting '{key}' from the command line is deprecated and "
"will be removed in ESMValCore version 2.4. For diagnostics "
"that support this setting, it should be set in the "
"diagnostic script section of the recipe instead.")
print(f"Warning: {msg}")
warnings.warn(DeprecationWarning(msg))

# set defaults
defaults = {
'write_plots': True,
Comment thread
bouweandela marked this conversation as resolved.
'write_netcdf': True,
'compress_netcdf': False,
'exit_on_warning': False,
'output_file_type': 'png',
Expand All @@ -63,6 +84,9 @@ def read_config_user_file(config_file, folder_name, options=None):
'profile_diagnostic': False,
'config_developer_file': None,
'drs': {},
# DEPRECATED: remove default settings below in v2.4
'write_plots': True,
'write_netcdf': True,
}

for key in defaults:
Expand Down
11 changes: 9 additions & 2 deletions esmvalcore/_recipe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1219,13 +1219,20 @@ def _initialize_scripts(self, diagnostic_name, raw_scripts,
for key in (
'output_file_type',

@stefsmeets stefsmeets Oct 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it is enough to just mark the parameters as deprecated in the config and raise a warning. I think it would be better to keep this part of the code 'as-is' for now. The checks to re-add the keys below adds clutter. Save these for a later patch, and apply it to remove the parameters from the entire codebase for 2.3.

@bouweandela bouweandela Oct 2, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The problem with keeping this as is, is that the parameters from config-user.yml then overwrite what it says in the recipe, and people might get confused that it doesn't work if they add the setting in the recipe

@stefsmeets stefsmeets Oct 2, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But if the warning is given for the config, then that should be enough in my opinion. Maybe it would be then a good idea to do a double check in the recipe runner and re-issue the warning there.

@bouweandela bouweandela Oct 5, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe it would be then a good idea to do a double check in the recipe runner and re-issue the warning there.

Do you mean here by the 'recipe runner'? Wouldn't that result in the same amount of code clutter, but less user friendly?

@stefsmeets stefsmeets Oct 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, I was thinking if that is the problem, we can also warn users as late as possible -- when the recipe is being executed by what I called the recipe runner 😅

I did not understand that the variables from the user config overwrite the recipe, which I guess is what added the complexity in the first place. Whatever is added here, we should make sure it is easy to undo.

'log_level',
'write_plots',
'write_netcdf',
'profile_diagnostic',
'auxiliary_data_dir',
):
settings[key] = self._cfg[key]

# Add deprecated settings from configuration file
# DEPRECATED: remove in v2.4
for key in (
'write_plots',
'write_netcdf',
):
if key not in settings and key in self._cfg:
settings[key] = self._cfg[key]

scripts[script_name] = {
'script': script,
'output_dir': settings['work_dir'],
Expand Down
11 changes: 7 additions & 4 deletions esmvalcore/_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,8 +377,6 @@ def _write_ncl_settings(self):
'work_dir',
'output_file_type',

@stefsmeets stefsmeets Oct 2, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here, don't delete these variables just yet and remove the block to add them back below.

@bouweandela bouweandela Oct 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

some change is needed here, because otherwise these settings will not be available as variable diag_script_info@write_plots etc, making it impossible to adjust diagnostic scripts to the new situtation before they are removed in 2.3. As an intermediate step, I thought I would make them available as both diag_script_info@write_plots and config_user_info@write_plots and then remove the latter in version 2.3.

'log_level',
'write_plots',
'write_netcdf',
}
settings = {'diag_script_info': {}, 'config_user_info': {}}
for key, value in self.settings.items():
Expand All @@ -389,6 +387,13 @@ def _write_ncl_settings(self):
else:
settings[key] = value

# Still add deprecated keys to config_user_info to avoid
# crashing the diagnostic script that need this.
# DEPRECATED: remove in v2.4
for key in ('write_plots', 'write_netcdf'):
if key in self.settings:
settings['config_user_info'][key] = self.settings[key]

write_ncl_settings(settings, filename)

return filename
Expand Down Expand Up @@ -560,9 +565,7 @@ def _collect_provenance(self):
'recipe',
'run_dir',
'version',
'write_netcdf',
'write_ncl_interface',
'write_plots',
'work_dir',
)
attrs = {
Expand Down
5 changes: 0 additions & 5 deletions esmvalcore/config-user.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,6 @@
###############################################################################
---

# Diagnostics create plots? [true]/false
write_plots: true
# Diagnostics write NetCDF files? [true]/false
write_netcdf: true
# Set the console log level debug, [info], warning, error
log_level: info
# Exit on warning (only for NCL diagnostic scripts)? true/[false]
exit_on_warning: false
Expand Down
7 changes: 3 additions & 4 deletions esmvalcore/experimental/config/_config_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,10 +251,8 @@ def deprecate(func, variable, version: str = None):

_validators = {
# deprecate in 2.2.0
'write_plots': deprecate(validate_bool, 'write_plots', '2.2.0'),
'write_netcdf': deprecate(validate_bool, 'write_netcdf', '2.2.0'),
'output_file_type': deprecate(validate_string, 'output_file_type',
'2.2.0'),
'write_plots': deprecate(validate_bool, 'write_plots', '2.4.0'),
'write_netcdf': deprecate(validate_bool, 'write_netcdf', '2.4.0'),

# From user config
'log_level': validate_string,
Expand All @@ -268,6 +266,7 @@ def deprecate(func, variable, version: str = None):
'config_developer_file': validate_config_developer,
'profile_diagnostic': validate_bool,
'run_diagnostic': validate_bool,
'output_file_type': validate_string,

# From CLI
"skip-nonexistent": validate_bool,
Expand Down
1 change: 0 additions & 1 deletion tests/integration/test_recipe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ def config_user(tmp_path):
filename = write_config_user_file(tmp_path)
cfg = esmvalcore._config.read_config_user_file(filename, 'recipe_test', {})
cfg['synda_download'] = False
cfg['output_file_type'] = 'png'
cfg['check_level'] = CheckLevels.DEFAULT
return cfg

Expand Down