moved execution and submission items from [task][job] to [task] #3480

wxtim · 2020-01-14T11:52:26Z

Part of the job platforms work:

A number of items currently held in the suite spec under runtime->task->job are to be moved to runtime->task.
These items are:

execution polling intervals,
execution retry delays,
execution time limit,
submission polling intervals,
submission retry delays

This PR

Moves these items in the config defined by cfgspec/suite.rc
Adds "upgraders" for each of these items in cfgspec/suite.rc so that users using Cylc7 syntax will get a warning message and have these items moved.
Adds a test for the deprecat

hjoliver

(looks good, but currently too tired to think straight ... I'll look closer tomorrow)

wxtim · 2020-01-15T12:00:30Z

(looks good, but currently too tired to think straight ... I'll look closer tomorrow)

it was 23:00 Local when you made that comment.

wxtim · 2020-01-16T14:58:40Z

cylc/flow/cfgspec/suite.py

@@ -149,6 +148,9 @@
    },
    'runtime': {
        '__MANY__': {
+            'execution polling intervals': [VDR.V_INTERVAL_LIST, None],


I've attempted to put these items in alphabetical order. Given the level of alphabetism in the previous version this was tricky.

Looked up "alphabetism", learned something new! http://www.glottopedia.org/index.php/Alphabetism (albeit not how you intended it, I guess).

wxtim · 2020-01-16T14:59:35Z

tests/deprecations/01-cylc8-basic.t

@@ -39,6 +39,11 @@ cmp_ok val.out <<__END__
 * (8.0.0) [cylc][reference test][suite shutdown event handler] - DELETED (OBSOLETE)
 * (8.0.0) [cylc][abort if any task fails] -> [cylc][events][abort if any task fails] - value unchanged
 * (8.0.0) [runtime][foo, cat, dog][job][shell] - DELETED (OBSOLETE)
+ * (8.0.0) [runtime][foo, cat, dog][job][execution polling intervals] -> [runtime][foo, cat, dog][execution polling intervals] - value unchanged


This test checks that the deprecations have worked.

wxtim · 2020-01-16T15:04:38Z

tests/jobscript/05-global-config.t

@@ -17,6 +17,9 @@
 #-------------------------------------------------------------------------------
 # Test that global config is used search for poll
 . "$(dirname "${0}")/test_header"
+
+# TODO - replace this with a Cylc 8 test
+skip_all "This test is broken pending full implementation of platforms"


We are not intending to have this sort of globalconfig work in Cylc 8. The best this test should give is a helpful warning, but that warning should already have been given elsewhere. More likely we need to re-write this in a platformsy way.

See issue #3481

fix test failing due to irrelevant upgrader warnign message config['job']['execution time limit'] --> config['execution time limit']

… for warning should just be in deprecations

…m -> [task]item

hjoliver · 2020-01-17T01:55:11Z

@wxtim -

A number of items currently held in the suite spec under runtime->task->job are to be moved to runtime->task.

Out of curiosity, was this flagged in one of the related proposals or issues? @oliver-sanders and I were trying to remember, but couldn't so trying to guess the reason for this. Presumably it is: some important job settings are moving to the new platforms config, and there's not enough left to bother keeping a jobs section in the suite config - is that right?

hjoliver

Code looks fine, but still interested in clarifying the motivation.

hjoliver · 2020-01-17T01:58:41Z

cylc/flow/cfgspec/suite.py

@@ -149,6 +148,9 @@
    },
    'runtime': {
        '__MANY__': {
+            'execution polling intervals': [VDR.V_INTERVAL_LIST, None],


Looked up "alphabetism", learned something new! http://www.glottopedia.org/index.php/Alphabetism (albeit not how you intended it, I guess).

hjoliver · 2020-01-17T02:04:10Z

Presumably it is: some important job settings are moving to the new platforms config, and there's not enough left to bother keeping a jobs section in the suite config - is that right?

I see from your global config PR, in the new job platforms section:

            # TODO ensure that it is possible to over-ride the following three
            # settings in suite config.
            'submission polling intervals': [VDR.V_INTERVAL_LIST],
            'submission retry delays': [VDR.V_INTERVAL_LIST, None],
            'execution polling intervals': [VDR.V_INTERVAL_LIST],

So that nixes my "not enough left to bother keeping a jobs section" argument.

(Sorry if I've just forgotten the motivating discussion for this change!).

hjoliver

Just un-approving again for clarity, because flakytests/events/01-task.t appears to be a genuine fail.

hjoliver · 2020-01-17T03:11:18Z

          # TODO ensure that it is possible to over-ride the following three
          # settings in suite config.

(Although in an offline chat with @oliver-sanders, maybe we don't want to allow this suite override)

wxtim · 2020-01-17T07:50:55Z

I'm concerned that I cannot find the reasoning behind this move. @dpmatthews can you remember. It was inserted, commented, in the original refactoring PR.
.

wxtim · 2020-01-17T08:48:48Z

Just un-approving again for clarity, because flakytests/events/01-task.t appears to be a genuine fail.

I wish there was a groan and 🙄 emoji! I'm so frustrated that I forgot to run these!

dpmatthews · 2020-01-17T11:40:04Z

I think the reason for this change is just to simplify the config. Does a [job] sub-section really make sense (especially now some items have been removed)?

I'm hoping we'll review the whole config at the workshop. I'm happy this change makes sense (and we can always change it again later) but I don't mind if others would prefer to delay.

oliver-sanders · 2020-01-20T23:57:17Z

Some users will find it amusing that we got them to move these items into the job section only a short while back, and now we are getting them to move them back out again!

TODO ensure that it is possible to over-ride the following three

Just to clarify, we aren't thinking of allowing users to override polling settings in a task are we?

hjoliver · 2020-01-21T00:15:15Z

TODO ensure that it is possible to over-ride the following three

Just to clarify, we aren't thinking of allowing users to override polling settings in a task are we?

I guess the question is: is there any reason to allow task-specific polling?

A valid use case?: users have to work around a specific important task that is known to fail silently sometimes. (Technically that's a bug in the task implementation, but fixing it might be "out of scope" or difficult.

hjoliver · 2020-01-21T00:21:38Z

Does a [job] sub-section really make sense (especially now some items have been removed)?

I agree IF "some items have been removed". But I think they haven't actually been removed, if we do allow task-specific override (see prev. comment).

dpmatthews · 2020-01-21T07:51:51Z

I think we have to allow task specific polling for the csae where you are using the polling task communication method. You might need to tune the polling depending on the expected runtime / queuing time for the task.

dpmatthews · 2020-01-21T14:44:17Z

Does a [job] sub-section really make sense (especially now some items have been removed)?

I agree IF "some items have been removed". But I think they haven't actually been removed, if we do allow task-specific override (see prev. comment).

runtime->task->job used to also contain batch system, batch submit command template and shell - these have all gone.

This PR isn't blocking anything so I suggest we put it on hold until we can discuss other potential config changes at the workshop.

hjoliver · 2020-01-21T23:16:34Z

OK, I forgot about batch system moving. It occurs to me, we probably do need task-specific batch system sometimes (e.g. for testing, or ... do we want to force people to use a proper batch system for trivial long-running polling jobs, for instance) ... but (trying to remember) we decided for that purpose you would be expected to define a different platform? ... (hpc_pbs and hpc_background...) which seems reasonable.

(slightly off-topic, sorry 😬 )

hjoliver · 2020-02-04T11:50:06Z

(Did you mean to close this @wxtim? Thought we were just putting it on hold for a bit)

wxtim · 2020-04-07T07:44:36Z

Closing this PR because it's going on hold for a bit. Can be re-opened when we are ready to deal but makes things tidier in the meantime.

wxtim changed the title ~~moved execution and submission items from [task][job] to [task]~~ [WIP] moved execution and submission items from [task][job] to [task] Jan 14, 2020

wxtim self-assigned this Jan 14, 2020

wxtim requested a review from datamel January 14, 2020 11:55

hjoliver reviewed Jan 15, 2020

View reviewed changes

wxtim changed the title ~~[WIP] moved execution and submission items from [task][job] to [task]~~ moved execution and submission items from [task][job] to [task] Jan 16, 2020

wxtim force-pushed the plumbing_platforms_simple_upgrades branch from 86185f7 to 1289ae2 Compare January 16, 2020 14:57

wxtim commented Jan 16, 2020

View reviewed changes

wxtim requested a review from oliver-sanders January 16, 2020 15:05

wxtim changed the title ~~moved execution and submission items from [task][job] to [task]~~ [Codacy Fail Only] moved execution and submission items from [task][job] to [task] Jan 16, 2020

wxtim added 3 commits January 16, 2020 15:16

moved execution and submission items from [task][job] to [task]

8cc899c

fix test failing due to irrelevant upgrader warnign message config['job']['execution time limit'] --> config['execution time limit']

refactoring examples of 'execution time limit' in test battery. Tests…

e32664b

… for warning should just be in deprecations

added deprecation tests for simple cylc-8 deprecations [task][job]ite…

a040fad

…m -> [task]item

wxtim force-pushed the plumbing_platforms_simple_upgrades branch from 1289ae2 to a040fad Compare January 16, 2020 15:16

oliver-sanders changed the title ~~[Codacy Fail Only] moved execution and submission items from [task][job] to [task]~~ moved execution and submission items from [task][job] to [task] Jan 16, 2020

Fix an error in test refactor

da8f188

oliver-sanders approved these changes Jan 17, 2020

View reviewed changes

hjoliver approved these changes Jan 17, 2020

View reviewed changes

hjoliver requested changes Jan 17, 2020

View reviewed changes

fixed a suite.rc syntax error in flakytests/events/01-task/suite.rc

053ea85

oliver-sanders mentioned this pull request Jan 29, 2020

build documentation into cylc.flow.cfgspec modules #3253

Closed

wxtim closed this Feb 4, 2020

wxtim reopened this Feb 4, 2020

wxtim closed this Apr 7, 2020

wxtim deleted the plumbing_platforms_simple_upgrades branch May 27, 2020 08:03

MetRonnie mentioned this pull request Apr 27, 2021

conf: ensure all proposed changes have been enacted #3696

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

moved execution and submission items from [task][job] to [task] #3480

moved execution and submission items from [task][job] to [task] #3480

wxtim commented Jan 14, 2020 •

edited

Loading

hjoliver left a comment

wxtim commented Jan 15, 2020

wxtim Jan 16, 2020

hjoliver Jan 17, 2020

wxtim Jan 16, 2020

wxtim Jan 16, 2020

hjoliver commented Jan 17, 2020

hjoliver left a comment

hjoliver Jan 17, 2020

hjoliver commented Jan 17, 2020 •

edited

Loading

hjoliver left a comment

hjoliver commented Jan 17, 2020

wxtim commented Jan 17, 2020

wxtim commented Jan 17, 2020

dpmatthews commented Jan 17, 2020 •

edited

Loading

oliver-sanders commented Jan 20, 2020

hjoliver commented Jan 21, 2020 •

edited

Loading

hjoliver commented Jan 21, 2020

dpmatthews commented Jan 21, 2020

dpmatthews commented Jan 21, 2020

hjoliver commented Jan 21, 2020

hjoliver commented Feb 4, 2020

wxtim commented Apr 7, 2020

moved execution and submission items from [task][job] to [task] #3480

moved execution and submission items from [task][job] to [task] #3480

Conversation

wxtim commented Jan 14, 2020 • edited Loading

This PR

hjoliver left a comment

Choose a reason for hiding this comment

wxtim commented Jan 15, 2020

wxtim Jan 16, 2020

Choose a reason for hiding this comment

hjoliver Jan 17, 2020

Choose a reason for hiding this comment

wxtim Jan 16, 2020

Choose a reason for hiding this comment

wxtim Jan 16, 2020

Choose a reason for hiding this comment

hjoliver commented Jan 17, 2020

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver Jan 17, 2020

Choose a reason for hiding this comment

hjoliver commented Jan 17, 2020 • edited Loading

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver commented Jan 17, 2020

wxtim commented Jan 17, 2020

wxtim commented Jan 17, 2020

dpmatthews commented Jan 17, 2020 • edited Loading

oliver-sanders commented Jan 20, 2020

hjoliver commented Jan 21, 2020 • edited Loading

hjoliver commented Jan 21, 2020

dpmatthews commented Jan 21, 2020

dpmatthews commented Jan 21, 2020

hjoliver commented Jan 21, 2020

hjoliver commented Feb 4, 2020

wxtim commented Apr 7, 2020

wxtim commented Jan 14, 2020 •

edited

Loading

hjoliver commented Jan 17, 2020 •

edited

Loading

dpmatthews commented Jan 17, 2020 •

edited

Loading

hjoliver commented Jan 21, 2020 •

edited

Loading