add runtime fields/deltas to def, proxy, job data elements #5138

dwsutherland · 2022-09-16T00:49:43Z

Includes runtime field for def, proxy, and jobs .. with:

def the task/family original workflow definition
proxy the task/family point version, that changes (deltas available) as soon as you broadcast
jobs, a live record of what they ran with.

example:

broadcasting:

cylc broadcast -s '[environment]GREETING = Broadcasting hello' '~sutherlander/linear/run1'

and then running foo again results in:

          {
            "id": "~sutherlander/linear/run1//20220901T00/foo",
            "state": "succeeded",
            "runtime": {
              "platform": "",
              "script": "sleep 2; echo \"$GREETING\"",
              "initScript": "echo 'Me first'",
              "envScript": "echo \"Hi first, I'm second\"",
              "errScript": "echo 'Boo!'",
              "exitScript": "echo 'Yay!'",
              "preScript": "sleep 1",
              "postScript": "sleep 1",
              "workSubDir": "",
              "executionTimeLimit": 0,
              "directives": {},
              "environment": {
                "GREETING": "Broadcasting hello"
              },
              "outputs": {}
            },
            "jobs": [
              {
                "id": "~sutherlander/linear/run1//20220901T00/foo/02",
                "state": "succeeded",
                "jobLogDir": "/home/sutherlander/cylc-run/linear/run1/log/job/20220901T00/foo/02",
                "runtime": {
                  "platform": "localhost",
                  "script": "sleep 2; echo \"$GREETING\"",
                  "initScript": "echo 'Me first'",
                  "envScript": "echo \"Hi first, I'm second\"",
                  "errScript": "echo 'Boo!'",
                  "exitScript": "echo 'Yay!'",
                  "preScript": "sleep 1",
                  "postScript": "sleep 1",
                  "workSubDir": "",
                  "executionTimeLimit": 0,
                  "directives": {},
                  "environment": {
                    "GREETING": "Broadcasting hello"
                  },
                  "outputs": {}
                }
              },
              {
                "id": "~sutherlander/linear/run1//20220901T00/foo/01",
                "state": "succeeded",
                "jobLogDir": "/home/sutherlander/cylc-run/linear/run1/log/job/20220901T00/foo/01",
                "runtime": {
                  "platform": "localhost",
                  "script": "sleep 2; echo \"$GREETING\"",
                  "initScript": "echo 'Me first'",
                  "envScript": "echo \"Hi first, I'm second\"",
                  "errScript": "echo 'Boo!'",
                  "exitScript": "echo 'Yay!'",
                  "preScript": "sleep 1",
                  "postScript": "sleep 1",
                  "workSubDir": "",
                  "executionTimeLimit": 0,
                  "directives": {},
                  "environment": {
                    "GREETING": "Hello from foo!"
                  },
                  "outputs": {}
                }
              }
            ]
          }

Cancel broadcast will create deltas on effected proxy nodes also.

Check List

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Applied any dependency changes to both setup.cfg and conda-environment.yml.
Tests are included (or explain why tests are not needed).
CHANGES.md entry included if this is a change that can affect users
Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
If this is a bug fix, PRs raised to both master and the relevant maintenance branch.

hjoliver · 2022-09-16T02:05:51Z

Did you mean to remove cylc/flow/data_messages_pb2.py ?

dwsutherland · 2022-09-16T02:10:34Z

Did you mean to remove cylc/flow/data_messages_pb2.py ?

It's not removed (nothing would work otherwise), I used the corresponding protoc:

$ protoc --version
libprotoc 3.19.4

It's just massively reduced.. like in:
#4901

(but not using that new version, so must have happened in protobuf==3.19.*)..

dwsutherland · 2022-09-16T02:16:36Z

cylc/flow/data_messages.proto

-    optional string env_script = 11;
-    optional string err_script = 12;
-    optional string exit_script = 13;
-    optional float execution_time_limit = 14;
    optional string platform = 15;


Left this in, so current API doesn't break.

Makes sense to keep this as the job platform is chosen based on the runtime configuration (e.g. platform could be set to a platform-group) causing these two values to differ.

dwsutherland · 2022-09-16T02:20:52Z

cylc/flow/task_job_mgr.py

@@ -617,40 +617,6 @@ def _create_job_log_path(workflow, itask):
                exc.filename = target
            raise exc

-    @staticmethod
-    def _get_job_scripts(itask, rtconfig):


This seemed to be a double up of what's already done in the config.py .. The only difference is the replacement of environmental variable CYLC_TASK_CYCLE_POINT for str(itask.point) (which isn't available earlier):

comstr = ( "cylc workflow-state " + " --task=" + itask.tdef.workflow_polling_cfg['task'] + " --point=" + str(itask.point) )

It's removal doesn't appear to have an impact, however, I could be wrong.. (is the env variable ever not available?)

CYLC_TASK_CYCLE_POINT should always be available in job execution environments so should be good.

Took a look and I think we are safe to remove this, cheers.

cylc/flow/data_messages.proto

MetRonnie · 2022-09-27T16:17:06Z

cylc/flow/data_store_mgr.py

+    try:
+        platform = rtconfig['platform']['name']
+    except (KeyError, TypeError):


Shouldn't rtconfig['platform'] just be a string of the platform name? In what circumstance would it be a dict containing a name key?

The confusing thing is.. Platform gets changed before job submission, both exist.

The dict version appears to be more common..

That is confusing. Is it just because one of your calls to runtime_from_config passes in the job config instead of the taskdef.rtconfig?

Ping @dwsutherland - one question to respond to here. As I recall, I didn't think it should be like this, hence my suggested reason for it.

Yes, the platform gets fully resolved/loaded on job construction (into a parsec dict).. however it's just a string in the workflow config runtime.

cylc/flow/network/schema.py

cylc/flow/scripts/broadcast.py

MetRonnie

Tested out in graphiql, looks good. Just noticed 1 typo

cylc/flow/network/schema.py

dwsutherland · 2022-09-29T01:31:53Z

This will need to be rebased once #4901 goes in (or visa versa)

(and protobuf module regenerated)

MetRonnie · 2022-10-03T13:57:55Z

Another problem Oliver has just realised: the order of the [environment] variables needs to be preserved, which we don't think it currently is by the GraphQL mechanism if using a dictionary as you've done at present

oliver-sanders · 2022-10-03T14:12:10Z

I think we'll need a list of environment variables to get around this e.g:

{
  "environment": [
    {"key": "answer", "value": 42}
  ]
}

dwsutherland · 2022-10-12T07:48:33Z

Ok I've fixed the problem.. I was using get() with ordered default dict, I changed to that as a workaround for some tests (so they might fail, and I'll just need to adjust the test(s))..

WRT the environment order:

Another problem Oliver has just realised: the order of the [environment] variables needs to be preserved, which we don't think it currently is by the GraphQL mechanism if using a dictionary as you've done at present

Dictionaries are ordered, and I dump them as string fields:

PbRuntime(
        platform=platform,
        script=rtconfig['script'],
        init_script=rtconfig['init-script'],
        env_script=rtconfig['env-script'],
        err_script=rtconfig['err-script'],
        exit_script=rtconfig['exit-script'],
        pre_script=rtconfig['pre-script'],
        post_script=rtconfig['post-script'],
        work_sub_dir=rtconfig['work sub-directory'],
        execution_time_limit=rtconfig['execution time limit'],
        directives=json.dumps(directives),
        environment=json.dumps(environment),
    )

The resolver does:

def resolve_json_dump(root, info, **args):
    field = getattr(root, to_snake_case(info.field_name), '{}') or '{}'
    return json.loads(field)

So unless something odd happens between this and firing it off to the client, the fields should be fine..

MetRonnie · 2022-10-12T13:33:48Z

I've had a look at [environment] order and confirmed the response order is identical to that which you get in log/config/NN-start-MM.cylc, including when inheritance is involved.

dwsutherland · 2022-10-17T06:31:10Z

Getting a KeyError on tests/functional/retries/01-submission-retry.t

Sorted now.. It's because some submission failures happen before full job construction.
(this is why I had changed it before, causing the field deletion due to parsec dictionary behavior with get())

oliver-sanders · 2022-10-18T10:48:40Z

WRT the environment order:

Another problem Oliver has just realised: the order of the [environment] variables needs to be preserved, which we don't think it currently is by the GraphQL mechanism if using a dictionary as you've done at present

Dictionaries are ordered, and I dump them as string fields:

Dictionaries are ordered in Python land, however, they are not in either JSON or JS. Because JSON is the transport format GraphQL uses and the UI code is JS I would not expect this order to be preserved.

So I think we need a JSON structure like this to preserve order:

[
  {
    "key": "FOO",
    "value": "42"
  },
  {
    "key": "BAR",
    "value": "answer"
  }
]

dwsutherland · 2022-11-11T00:57:46Z

Ok, I've put a workaround in that will check and alter 8.0.x broadcast db loads by;

creating a DurationFloat out of the seconds float string.
strip the loaded list-strings of [ ], if they both exist on the respective ends. (this is narrowed down to only list settings in the config)

here's an old and new (via a couple of restarts):

2022-11-11T00:44:45Z INFO - + [*/foo] execution retry delays=[120.0, 180.0]
2022-11-11T00:44:45Z INFO - + [*/foo] submission retry delays=PT2M, PT3M

This workaround can be removed some time in the future...

dwsutherland · 2022-11-11T06:30:29Z

(most the missing coverage is pre-existing, and just replicated-in/copied-to the workaround)

MetRonnie · 2022-11-11T11:36:24Z

I have opened a PR against this with a functional test for the restart: dwsutherland#10

oliver-sanders · 2022-11-14T10:31:44Z

cylc/flow/task_job_mgr.py

@@ -617,40 +617,6 @@ def _create_job_log_path(workflow, itask):
                exc.filename = target
            raise exc

-    @staticmethod
-    def _get_job_scripts(itask, rtconfig):


CYLC_TASK_CYCLE_POINT should always be available in job execution environments so should be good.

Took a look and I think we are safe to remove this, cheers.

tests/functional/database/08-broadcast-upgrade/flow.cylc

cylc/flow/parsec/validate.py

oliver-sanders

LGTM, tested runtime fields on the following objects:

Task (task config as defined in flow.cylc, no broadcasts applied)
TaskProxy (task config + broadcasts)
Family (family config as defined in flow.cylc, no broadcasts applied)
FamilyProxy (family config + broadcasts)
Job (task config + broadcasts at the time of job submission, no backsies)

All worked as expected 🚀

I spotted two minor issues during the course of testing...

1) Duration lists get formatted as integers when cleared.

When I create a broadcast it is presented to me in native-ISO8601 format:

$ cylc broadcast broad -s 'execution retry delays = PT1M'
Broadcast set:
+ [*/root] execution retry delays=PT1M

But when I clear the broadcast it is presented to me in integer format:

$ cylc broadcast --clear broad
Broadcast cancelled:
- [*/root] execution retry delays=[60.0]

2) Updated deltas contain all fields

E.G. for this GraphQL query:

subscription {
  deltas {
    updated(stripNull: true) {
      taskProxies {
        id
        runtime {
          script
          executionRetryDelays
        }
      }
    }
  }
}

And the following broadcast:

$ cylc broadcast broad -s 'execution retry delays = PT10M'
Broadcast set:
+ [*/root] execution retry delays=PT10M

I get an updated delta like this:

{
  "data": {
    "deltas": {
      "updated": {
        "taskProxies": [
          {
            "id": "~osanders/broad//20191209T1200Z/pub",
            "runtime": {
              "script": "sleep 1",
              "executionRetryDelays": "PT10M"
            }
          },
          {
            "id": "~osanders/broad//20191209T1200Z/wipe_bar",
            "runtime": {
              "script": "sleep 1",
              "executionRetryDelays": "PT10M"
            }
          },

Which contains the execution retry delays which have changed but also the script which hasn't.

As long as this doesn't cause broadcasted runtime fields to get re-sent for things like task state changes this is probably harmless.

Co-authored-by: Ronnie Dutta <[email protected]>

Co-authored-by: Oliver Sanders <[email protected]>

dwsutherland · 2022-11-16T05:58:33Z

Duration lists get formatted as integers when cleared.

Fixed.. Internal objects/format needed stringified for response.

Updated deltas contain all fields

Yes, it's not fine grained at the moment.. It would be a tricky thing to do, as some have been added or cleared (and then there's inheritance).. I filter out those runtimes whose serialized form hasn't changed.

oliver-sanders

Thanks @dwsutherland 🚀

dwsutherland self-assigned this Sep 16, 2022

dwsutherland changed the title ~~add runtime fields/deltas to def, proxy, jobs~~ add runtime fields/deltas to def, proxy, job data elements Sep 16, 2022

dwsutherland requested a review from oliver-sanders September 16, 2022 00:52

dwsutherland force-pushed the runtime-api-fields branch from 3e0a277 to bdf490c Compare September 16, 2022 02:00

dwsutherland commented Sep 16, 2022

View reviewed changes

dwsutherland force-pushed the runtime-api-fields branch 3 times, most recently from a18042d to a95d9be Compare September 16, 2022 04:33

oliver-sanders added this to the cylc-8.1.0 milestone Sep 21, 2022

MetRonnie self-requested a review September 27, 2022 15:59

MetRonnie reviewed Sep 27, 2022

View reviewed changes

MetRonnie reviewed Sep 28, 2022

View reviewed changes

cylc/flow/network/schema.py Outdated Show resolved Hide resolved

MetRonnie approved these changes Sep 28, 2022

View reviewed changes

dwsutherland force-pushed the runtime-api-fields branch from 79b2973 to ccd2bab Compare September 30, 2022 13:48

MetRonnie approved these changes Sep 30, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

dwsutherland force-pushed the runtime-api-fields branch from ccd2bab to 3351159 Compare October 12, 2022 07:41

This comment was marked as resolved.

Sign in to view

dwsutherland force-pushed the runtime-api-fields branch from 3351159 to 765b1d7 Compare October 17, 2022 03:17

dwsutherland force-pushed the runtime-api-fields branch 2 times, most recently from a9b8c07 to 18aa91a Compare November 11, 2022 00:49

dwsutherland force-pushed the runtime-api-fields branch from 18aa91a to 20e04fb Compare November 11, 2022 01:25

MetRonnie mentioned this pull request Nov 11, 2022

Add test & tidy dwsutherland/cylc-flow#10

Merged

oliver-sanders reviewed Nov 14, 2022

View reviewed changes

MetRonnie mentioned this pull request Nov 14, 2022

Response to Oliver's review dwsutherland/cylc-flow#11

Merged

oliver-sanders reviewed Nov 14, 2022

View reviewed changes

dwsutherland and others added 13 commits November 16, 2022 17:30

add runtime fields/deltas to def, proxy, jobs

67623dc

Update schema runtime description

e959fe9

Co-authored-by: Ronnie Dutta <[email protected]>

typo in schema runtime field

37e5ca1

Co-authored-by: Ronnie Dutta <[email protected]>

fix defaults dic lookup

cc74687

runtime dict to ordered array

fb3d632

stringify runtime float lists

3f4cc4d

broadcast external vs internal format coercion

d1d0681

Workaround for 8.0.x to 8.1.0 broadcast DB loads

3084eaf

Add test & tidy

0f491f4

Update cylc/flow/parsec/validate.py

c8fbfa4

Co-authored-by: Oliver Sanders <[email protected]>

Update tests

b60e878

Add back compat notices as per review suggestion

cadb55f

clear broadcast reported value format

eb1382a

dwsutherland force-pushed the runtime-api-fields branch from a522566 to eb1382a Compare November 16, 2022 04:31

MetRonnie approved these changes Nov 17, 2022

View reviewed changes

oliver-sanders approved these changes Nov 18, 2022

View reviewed changes

oliver-sanders merged commit 90c3c3c into cylc:master Nov 18, 2022

MetRonnie mentioned this pull request Nov 18, 2022

Enable broadcast from UI Edit-Runtime form #5232

Merged

7 tasks

MetRonnie added the schema change Change to the Cylc GraphQL schema label Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add runtime fields/deltas to def, proxy, job data elements #5138

add runtime fields/deltas to def, proxy, job data elements #5138

dwsutherland commented Sep 16, 2022 •

edited

Loading

hjoliver commented Sep 16, 2022

dwsutherland commented Sep 16, 2022 •

edited

Loading

dwsutherland Sep 16, 2022

oliver-sanders Nov 14, 2022

dwsutherland Sep 16, 2022 •

edited

Loading

oliver-sanders Nov 14, 2022

MetRonnie Sep 27, 2022

dwsutherland Sep 28, 2022

hjoliver Sep 29, 2022

hjoliver Oct 17, 2022

dwsutherland Oct 18, 2022 •

edited

Loading

MetRonnie left a comment

dwsutherland commented Sep 29, 2022 •

edited

Loading

This comment was marked as resolved.

MetRonnie commented Oct 3, 2022

oliver-sanders commented Oct 3, 2022 •

edited

Loading

This comment was marked as resolved.

dwsutherland commented Oct 12, 2022

MetRonnie commented Oct 12, 2022

This comment was marked as resolved.

dwsutherland commented Oct 17, 2022

oliver-sanders commented Oct 18, 2022 •

edited

Loading

dwsutherland commented Nov 11, 2022 •

edited

Loading

dwsutherland commented Nov 11, 2022 •

edited

Loading

MetRonnie commented Nov 11, 2022

oliver-sanders Nov 14, 2022

oliver-sanders left a comment

dwsutherland commented Nov 16, 2022 •

edited

Loading

oliver-sanders left a comment

add runtime fields/deltas to def, proxy, job data elements #5138

add runtime fields/deltas to def, proxy, job data elements #5138

Conversation

dwsutherland commented Sep 16, 2022 • edited Loading

hjoliver commented Sep 16, 2022

dwsutherland commented Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwsutherland Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwsutherland Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

dwsutherland commented Sep 29, 2022 • edited Loading

This comment was marked as resolved.

MetRonnie commented Oct 3, 2022

oliver-sanders commented Oct 3, 2022 • edited Loading

This comment was marked as resolved.

dwsutherland commented Oct 12, 2022

MetRonnie commented Oct 12, 2022

This comment was marked as resolved.

dwsutherland commented Oct 17, 2022

oliver-sanders commented Oct 18, 2022 • edited Loading

dwsutherland commented Nov 11, 2022 • edited Loading

dwsutherland commented Nov 11, 2022 • edited Loading

MetRonnie commented Nov 11, 2022

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

1) Duration lists get formatted as integers when cleared.

2) Updated deltas contain all fields

dwsutherland commented Nov 16, 2022 • edited Loading

oliver-sanders left a comment

Choose a reason for hiding this comment

dwsutherland commented Sep 16, 2022 •

edited

Loading

dwsutherland commented Sep 16, 2022 •

edited

Loading

dwsutherland Sep 16, 2022 •

edited

Loading

dwsutherland Oct 18, 2022 •

edited

Loading

dwsutherland commented Sep 29, 2022 •

edited

Loading

oliver-sanders commented Oct 3, 2022 •

edited

Loading

oliver-sanders commented Oct 18, 2022 •

edited

Loading

dwsutherland commented Nov 11, 2022 •

edited

Loading

dwsutherland commented Nov 11, 2022 •

edited

Loading

dwsutherland commented Nov 16, 2022 •

edited

Loading