Add missing value cycling_mode to cylc.flags

[kinow]

While review #2809 and reading the code of scheduler.py in the IDE, I noticed it couldn't locate cycling_mode in flags.py. Even though it is missing, it is defined in config.py's SuiteConfig constructor.

Might be worth adding the empty value by default for clarity?

[matthewrmshin]

Hi @kinow Sorry, but I really don't like these global flags. Given that the flag is read in 2 locations (and is written in 1 location), I would like to propose for it to be removed 💥:

diff --git a/bin/cylc-submit b/bin/cylc-submit
index 4a67e1ac1..d502ebac1 100755
--- a/bin/cylc-submit
+++ b/bin/cylc-submit
@@ -116,7 +116,7 @@ def main():
         'CYLC_DEBUG': str(cylc.flags.debug).lower(),
         'CYLC_VERBOSE': str(cylc.flags.verbose).lower(),
         'CYLC_SUITE_NAME': suite,
-        'CYLC_CYCLING_MODE': str(cylc.flags.cycling_mode),
+        'CYLC_CYCLING_MODE': str(config.cfg['scheduling']['cycling mode']),
         'CYLC_SUITE_INITIAL_CYCLE_POINT': str(
             config.cfg['scheduling']['initial cycle point']),
         'CYLC_SUITE_FINAL_CYCLE_POINT': str(
diff --git a/lib/cylc/config.py b/lib/cylc/config.py
index da3af892e..2358d33b8 100644
--- a/lib/cylc/config.py
+++ b/lib/cylc/config.py
@@ -339,8 +339,6 @@ class SuiteConfig(object):
             set_utc_mode(glbl_cfg().get(['cylc', 'UTC mode']))
         else:
             set_utc_mode(self.cfg['cylc']['UTC mode'])
-        # Capture cycling mode
-        cylc.flags.cycling_mode = self.cfg['scheduling']['cycling mode']
 
         # Initial point from suite definition (or CLI override above).
         icp = self.cfg['scheduling']['initial cycle point']
diff --git a/lib/cylc/scheduler.py b/lib/cylc/scheduler.py
index a9bb75388..942ea9a2c 100644
--- a/lib/cylc/scheduler.py
+++ b/lib/cylc/scheduler.py
@@ -1105,7 +1105,8 @@ conditions; see `cylc conditions`.
             'CYLC_DEBUG': str(cylc.flags.debug).lower(),
             'CYLC_VERBOSE': str(cylc.flags.verbose).lower(),
             'CYLC_SUITE_NAME': self.suite,
-            'CYLC_CYCLING_MODE': str(cylc.flags.cycling_mode),
+            'CYLC_CYCLING_MODE': str(
+                self.config.cfg['scheduling']['cycling mode']),
             'CYLC_SUITE_INITIAL_CYCLE_POINT': str(self.initial_point),
             'CYLC_SUITE_FINAL_CYCLE_POINT': str(self.final_point),
         })

[kinow]

On mobile now. Also not very fond of mutable globals like this. Removing sounds a bit drastic, but I'd be OK with that option too.

Should we get a 3rd opinion here? I'm OK with both options.

Cheers

[matthewrmshin]

No problem. Assigned @hjoliver for a final say. 😄

Let's also block 🚫 the next release with this PR - as current master is a bit unsafe ❗ without the global flag 🎏 being defined.

Note on removal:

• It is actually not too bad, given that this flag is only used in 2 locations - and really just to set an environment variable - and in both cases, the flag is really duplicating a value held by the configuration object.
• My recent logging related changes are attempts to reduce the usage of the cylc.flags.verbose and cylc.flags.debug flags. The former is almost eliminated. The latter is still widespread.
• The remaining is cylc.flags.iflag. Its usage is not widespread, but getting rid of it will require turning some logic upside down in cylc.task_state, so I'll wait until we have time to tackle e.g. Improve interaction between task state, output and prerequisite #2329.
• (We used to have cylc.flags.pflag as well. This has been moved to become an instance attribute of cylc.task_events_mgr.TaskEventsManager.)

[hjoliver]

Obviously I agree that any non-trivial use of global variables is bad, but for a small number of simple "constants" (quotes for Python constants) that are more or less globally used - like verbosity and debug flags - I'm not sure that the standard objections really mean much.

The iflag one does reflect an old design flaw that we haven't addressed (my fault).

I'm not sure why cycling_mode is there, but there seems to be no good reason at all to have it as a global, so I vote for removing it as per Matt's diff above - seems safe to me.

[hjoliver]

(commented already)

[kinow]

@matthewrmshin feel free to open a PR with the code from your diff above for this. Then I will close this one, and can also help review/testing that one. WDYT?

[matthewrmshin]

Superseded by #2869.