Describe the bug
A user was complaining of not getting configured event emails. His suite config contains both the ancient [event hooks] section deprecated in 6.11.0 as well as the newer [events]mail events.
Release version(s) and/or repository branch(es) affected?
cylc-7.8.4
Steps to reproduce the bug
- configure some task mail events:
#...
[runtime]
[[root]]
[[[events]]]
mail to = [email protected]
mail events = succeeded, failed
# ...
Result as expected:
$ cylc get-config --sparse -i '[runtime][foo][events]' suite.rc
mail events = succeeded, failed
mail to = [email protected]
- Add in the old event hooks section as well:
[[[event hooks]]]
failed handler = oops.py
Result, the old config gets upgraded but the new mail config gets wiped out:
$ cylc get-config --sparse -i '[runtime][foo][events]' suite.rc
2020-02-04T23:25:56Z WARNING - deprecated items were automatically upgraded in 'suite definition':
2020-02-04T23:25:56Z WARNING - * (6.11.0) [runtime][root][event hooks] -> [runtime][root][events] - value unchanged
failed handler = oops.py
Expected behavior
Arguably using deprecated items as well as their replacements in the same suite is a recipe for problems, but ideally the new config should not get wiped out by the upgraded old config. We should warn or fail validation if there is a clash between the old and the new.
Pull requests welcome!
This is an Open Source project - please consider contributing a bug fix
yourself (please read CONTRIBUTING.md before starting any work though).
(This particular config combo won't be a problem on master because we've obsoleted the 6.11.0 changes for Cylc 8 ... but the same problem might occur for other deprecation upgrades).
hjoliver
All 10 comments
(Should probably be fixed for 7.8.x as well as master).
hjoliver
on 5 Feb 2020
The event hooks appears to have been removed in Cylc 8, so it doesn't pass validation.
(venv) kinow@kinow-VirtualBox:~/cylc-run/3494$ cylc get-config --sparse -i '[runtime][root][events]' suite.rc
IllegalItemError: [runtime][root]event hooks
But on 7.8.x the issue is easy to be reproduced with your examples. Would an obsolete instead of deprecate call work here?
diff --git a/lib/cylc/cfgspec/globalcfg.py b/lib/cylc/cfgspec/globalcfg.py
index 38c66249d..4e3a41aaf 100644
--- a/lib/cylc/cfgspec/globalcfg.py
+++ b/lib/cylc/cfgspec/globalcfg.py
@@ -274,12 +274,12 @@ def upg(cfg, descr):
u.obsolete(
'6.11.0',
['state dump rolling archive length'])
- u.deprecate(
+ u.obsolete(
'6.11.0',
['cylc', 'event hooks'],
['cylc', 'events'])
for key in SPEC['cylc']['events']:
- u.deprecate(
+ u.obsolete(
'6.11.0',
['cylc', 'event hooks', key],
['cylc', 'events', key])
diff --git a/lib/cylc/cfgspec/suite.py b/lib/cylc/cfgspec/suite.py
index 278be8efe..a9ba21bbf 100644
--- a/lib/cylc/cfgspec/suite.py
+++ b/lib/cylc/cfgspec/suite.py
@@ -310,9 +310,9 @@ def upg(cfg, descr):
['scheduling', 'special tasks', 'external-triggered'],
['scheduling', 'special tasks', 'external-trigger'],
)
- u.deprecate(
+ u.obsolete(
'6.11.0', ['cylc', 'event hooks'], ['cylc', 'events'])
- u.deprecate(
+ u.obsolete(
'6.11.0',
['runtime', '__MANY__', 'event hooks'],
['runtime', '__MANY__', 'events'])
That way instead of replacing the old value, running the command gives:
kinow@kinow-VirtualBox:~/cylc-run/3494$ cylc get-config --sparse -i '[runtime][root][events]' suite.rc
2020-03-16T15:31:44+13:00 WARNING - deprecated items were automatically upgraded in 'suite definition':
2020-03-16T15:31:44+13:00 WARNING - * (6.11.0) [runtime][root][event hooks] -> [runtime][root][events] - DELETED (OBSOLETE)
mail to = [email protected]
mail events = succeeded, failed
kinow@kinow-VirtualBox:~/cylc-run/3494$
kinow@kinow-VirtualBox:~/cylc-run/3494$ cylc validate suite.rc
WARNING - deprecated items were automatically upgraded in 'suite definition':
WARNING - * (6.11.0) [runtime][root][event hooks] -> [runtime][root][events] - DELETED (OBSOLETE)
Valid for cylc-7.8.4-20-ge4028-dirty
Users would have the new key=value, and a warning informing that the old key was deleted. I think... Or otherwise a new upgrader that raises an error when that key is found, and validation fails.
kinow
on 16 Mar 2020
Hi all, I have tried tackling this as my first issue working at the Met Office, as suggested by Tim. I've gone for the approach of raising an error instead of a warning.
In lib/parsec/upgrade.py it now checks for a clash between deprecated and existing non-deprecated item.
diff --git a/lib/parsec/upgrade.py b/lib/parsec/upgrade.py
index c293b5ecd..7722e497b 100755
--- a/lib/parsec/upgrade.py
+++ b/lib/parsec/upgrade.py
@@ -176,7 +176,21 @@ class upgrader(object):
warnings[vn].append(msg)
self.del_item(upg['old'])
if upg['cvt'].describe() != "DELETED (OBSOLETE)":
- self.put_item(upg['new'], upg['cvt'].convert(old))
+ # check self.cfg does not already contain a
+ # non-deprecated item matching upg['new']:
+ try:
+ self.get_item(upg['new'])
+ except KeyError:
+ self.put_item(upg['new'],
+ upg['cvt'].convert(old))
+ else:
+ errMsg = (
+ 'ERROR: Cannot upgrade '
+ 'deprecation "' + msg + '" because '
+ 'deprecated section clashes with existing '
+ 'non-deprecated section'
+ )
+ LOG.error(errMsg)
if warnings:
level = WARNING
if self.descr == self.SITE_CONFIG:
MetRonnie
on 16 Mar 2020
@kinow -
The event hooks appears to have been removed in Cylc 8, so it doesn't pass validation.
Yes, that agrees with my final comment in the description.
Would an obsolete instead of deprecate call work here?
It looks like it would, but only for items that have actually been obsoleted. I think we still need to fix the general problem, which is: don't upgrade a deprecated item to its new equivalent if the user has also set the new item. Looks like @MetRonnie has addressed this though ...
hjoliver
on 18 Mar 2020
This is fixed on 7.8.x and 7.9.x now. On master (Cylc 8) I get
$ cylc validate .
WARNING - deprecated graph items were automatically upgraded in 'suite definition':
WARNING - * (8.0.0) [scheduling][dependencies][X][graph] -> [scheduling][graph][X] - for X in:
P1D
IllegalItemError: [runtime][foo, cat, dog]event hooks
I assume support for upgrading the deprecated event hooks has ended in cylc 8?
MetRonnie
on 1 Apr 2020
[Edited - I'm losing my mind] We have the same problem in 8:
Consider the suite.rc
[scheduling]
initial cycle point = 20150808T00
# Deprecated:
[[dependencies]]
[[[P1D]]]
graph = bar => horse
# New in Cylc 8:
[[graph]]
P1D = foo => cat & dog
Then
$ cylc get-config --sparse -i '[scheduling]' suite.rc
2020-04-01T16:38:47+01:00 WARNING - deprecated graph items were automatically upgraded in 'suite definition':
2020-04-01T16:38:47+01:00 WARNING - * (8.0.0) [scheduling][dependencies][X][graph] -> [scheduling][graph][X] - for X in:
P1D
initial cycle point = 20150808T00
[[graph]]
P1D = bar => horse
Yes I expect we do have exactly the same problem on master.
hjoliver
on 1 Apr 2020
Changing the milestone to 8 since fixed on 7.8,9
hjoliver
on 1 Apr 2020
Actually the code responsible for upgrading the old style [dependencies][P1D]graph -> [graph]P1D is in a different file to the main upgrader:
https://github.com/cylc/cylc-flow/blob/0a95c0080b77235a6a377845ccf24ee927314f1e/cylc/flow/cfgspec/suite.py#L326-L349
MetRonnie
on 2 Apr 2020
Created PR #3549 for the master branch
MetRonnie
on 2 Apr 2020
Related issues
kinow
路
4Comments
sadielbartholomew
路
4Comments
sadielbartholomew
路
4Comments
kinow
路
4Comments
kinow
路
3Comments
Most helpful comment
Hi all, I have tried tackling this as my first issue working at the Met Office, as suggested by Tim. I've gone for the approach of raising an error instead of a warning.
In
lib/parsec/upgrade.pyit now checks for a clash between deprecated and existing non-deprecated item.