events: refactor key and make the event message available to handlers #5769

oliver-sanders · 2023-10-12T16:39:37Z

#5566 turned out to be remarkably awkward to solve, as a result there are two other changes on this PR.

Tokens objects are now immutable.
- Partially addresses cache Tokens methods #5183
- This means we can use them as dict keys and in sets which is greatly advantageous.
- It also opens the door to caching as needed.
Task events now use a named tuple and the fields have been rationalised.
- Closes event emails should include associated task messages #5566
- The separate cycle/task/job fields have been combined into tokens.
- The key1 field has been flattened out.
The message field is now available to event handlers.
- This means we can get access to task messages.
- There is a caveat attached to this, I haven't added a message column to the DB. This would be awkward as the table is shared with task action timers, etc.
- This means that if an event handler is actioned just before a workflow shuts down, the exact message will be lost when the workflow restarts.
- In this situation, it defaults to the event name, which might be INFO or custom for a task message.
Fix a remote test which doesn't get run on CI and became broken by another change.
Fix an email field which was never present.
Change the event email format a tad to allow multiple event messages per email.

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 if present).
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, PR should be raised against the relevant ?.?.x branch.

oliver-sanders · 2023-10-18T12:42:51Z

.github/workflows/test_functional.yml

@@ -108,7 +108,7 @@ jobs:
        run: |
          # install system deps
          brew update
-          brew install bash coreutils gnu-sed
+          brew install bash coreutils gnu-sed grep


Install GNU grep.

I think this may not work out of the box: You will probably have to do some path wrangling as well to make grep be ggrep rather than the BSD grep native to macos.

When I tried this however I got tonnes of errors: https://github.com/cylc/cylc-flow/actions/runs/6863292558/job/18662797573?pr=5819

It might be better if you do this since you have a local mac to try this stuff out on.

oliver-sanders · 2023-10-23T15:44:33Z

(draft until I can confirm there are no glaring coverage holes)

cylc/flow/id.py

dwsutherland

LGTM 👍 .. (MacOS test pending, of course)

dwsutherland · 2023-11-22T04:51:59Z

cylc/flow/id.py

@@ -159,6 +163,12 @@ def __eq__(self, other):
            for key in self._KEYS
        )

+    def __lt__(self, other):
+        return self.id < other.id


Trying to figure out the utility of this beyond different cycles or submissions of the same task.. I guess I'll find out (?), maybe a requirement for use in dicts or sets?

I think something was sorting tokens somewhere, possibly to get cycles/tasks in order in emails.

oliver-sanders · 2024-01-03T16:30:35Z

cylc/flow/task_events_mgr.py

-        for key in (
-            WorkflowEventData.Workflow.value,
-            WorkflowEventData.Host.value,
-            WorkflowEventData.Port.value,


Fix a small bug where the port was never provided to the event handler as it had changed from Scheduler.port to Scheduler.server.port.

oliver-sanders · 2024-01-03T16:32:54Z

cylc/flow/task_events_mgr.py

+    tokens: 'Tokens'
+
+
+def get_event_id(event: str, itask: 'TaskProxy') -> str:


This was pre-existing code which I centralised.

oliver-sanders · 2024-01-03T16:35:25Z

cylc/flow/task_events_mgr.py

+            stdin_str += f'job: {id_key.tokens.relative_id}\n'
+            stdin_str += f'event: {id_key.event}\n'
+            stdin_str += f'message: {id_key.message}\n\n'


Add the message into the email.

Split the oneline email template into multiple lines with a blank line between events.

Will check, but both the source and the output I'm expecting look more readable.

* Having Tokens objects mutable seemed like a good idea at the time. * But in practice, Tokens are rarely modified. * And their mutability is a barrier to use as dictionary keys and caching. * Implement the `__hash__` interface and block the `__setitem__` and `update` interfaces.

* Use a named tuple to store task event keys. * Rationalise the fields of task event keys. * Centralise event ID code. * Fixes an unrelated bug where the workflow port was not being included in emails.

* Closes cylc#5566 * Note, the event message is not stored in the database, so when events are restored from the DB on restart, the event message will default to the event name.

* Test had become broken by a positive behaviour change.

wxtim · 2024-01-04T10:39:33Z

98% Happy

Discussed example

#!jinja2
[meta]
    description = This is a workflow created by a script..
    created on = 2024-1-4T9:58

[scheduler]
    allow implicit tasks = True
    cycle point format = %Y
    [[events]]
        mail events = warning
    [[mail]]
        footer = The event was %(event)s The message was \n%(message)s

[scheduling]
    initial cycle point = 2469
    # final cycle point = 2469
    [[graph]]
        R1 = foo

[runtime]
    [[root]]
        script = """
            cylc message -- WARNING:"some information"
            exit 0
        """
        [[[events]]]
        #     mail events = warning
        #     [scheduler]
        warning handlers = my-handler %(workflow)s %(event)s %(message)s
        # platform

with

#!/bin/bash
#workflow/bin/my-handler

touch ${HOME}/info
echo "Workflow $1 event message: $2:$3" >> ${HOME}/info

In this case you don't get a message in the warning but you do get the event.

hjoliver · 2024-01-16T00:27:20Z

98% Happy

@wxtim & @oliver-sanders - is there still something to finish off on this?

hjoliver

LGTM

wxtim · 2024-01-16T08:45:20Z

98% Happy

@wxtim & @oliver-sanders - is there still something to finish off on this?

The example above - Oliver was going to have a think about it. TBH this is a massive improvement.

oliver-sanders · 2024-01-16T09:57:40Z

@wxtim

Latest commit makes the message available to the scheduler mail footer.

Note entirely sure this makes sense (there may be multiple events/messages, the current logic just uses the last one), but this makes it consistent with the use of "event".

Note: lint failures fixed on master

wxtim

Looks like the best for the moment. Are you happy with 1 review for the last commit?

oliver-sanders · 2024-01-17T11:23:41Z

It's trivial (pass the message with the event), I think we're good.

Lint failures are from master.

oliver-sanders added this to the cylc-8.x milestone Oct 12, 2023

oliver-sanders self-assigned this Oct 12, 2023

oliver-sanders force-pushed the id_key branch 2 times, most recently from c73a5cc to 4ef5109 Compare October 16, 2023 12:19

oliver-sanders commented Oct 18, 2023

View reviewed changes

oliver-sanders marked this pull request as ready for review October 23, 2023 15:43

oliver-sanders marked this pull request as draft October 23, 2023 15:43

oliver-sanders mentioned this pull request Nov 8, 2023

Support Python 3.12 #5794

Merged

8 tasks

wxtim reviewed Nov 14, 2023

View reviewed changes

cylc/flow/id.py Outdated Show resolved Hide resolved

dwsutherland approved these changes Nov 22, 2023

View reviewed changes

oliver-sanders force-pushed the id_key branch from 4ef5109 to b739ee5 Compare January 2, 2024 16:17

oliver-sanders commented Jan 3, 2024

View reviewed changes

oliver-sanders added 4 commits January 3, 2024 16:45

task_events_mgr: refactor event keys

2fbcf49

* Use a named tuple to store task event keys. * Rationalise the fields of task event keys. * Centralise event ID code. * Fixes an unrelated bug where the workflow port was not being included in emails.

task_events_mgr: include event message with event emails

01d6b8f

* Closes cylc#5566 * Note, the event message is not stored in the database, so when events are restored from the DB on restart, the event message will default to the event name.

tests/f: fix restart/44

a4976e1

* Test had become broken by a positive behaviour change.

oliver-sanders force-pushed the id_key branch from b739ee5 to a4976e1 Compare January 3, 2024 16:45

oliver-sanders marked this pull request as ready for review January 3, 2024 17:01

oliver-sanders requested a review from wxtim January 3, 2024 17:01

This was referenced Jan 4, 2024

add a profiler cylc/cylc-uiserver#546

Merged

efficiency: investigate bottleneck cylc/cylc-uiserver#547

Open

resolvers: optimize "uniq" iteration #5914

Merged

hjoliver approved these changes Jan 16, 2024

View reviewed changes

task message: provide message to scheduler mail footer

615013b

wxtim approved these changes Jan 17, 2024

View reviewed changes

oliver-sanders merged commit 90cba23 into cylc:master Jan 17, 2024
26 of 27 checks passed

oliver-sanders deleted the id_key branch January 17, 2024 11:24

oliver-sanders modified the milestones: cylc-8.x, cylc-8.3.0 Jan 22, 2024

MetRonnie mentioned this pull request Jun 10, 2024

Tokens are not properly immutable #6129

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: refactor key and make the event message available to handlers #5769

events: refactor key and make the event message available to handlers #5769

oliver-sanders commented Oct 12, 2023 •

edited

Loading

oliver-sanders Oct 18, 2023

wxtim Nov 14, 2023 •

edited

Loading

oliver-sanders commented Oct 23, 2023

dwsutherland left a comment •

edited

Loading

dwsutherland Nov 22, 2023

oliver-sanders Nov 22, 2023

oliver-sanders Jan 3, 2024

oliver-sanders Jan 3, 2024

oliver-sanders Jan 3, 2024

wxtim Jan 4, 2024

wxtim commented Jan 4, 2024 •

edited

Loading

hjoliver commented Jan 16, 2024

hjoliver left a comment

wxtim commented Jan 16, 2024

oliver-sanders commented Jan 16, 2024 •

edited

Loading

wxtim left a comment •

edited

Loading

oliver-sanders commented Jan 17, 2024 •

edited

Loading

		tokens: 'Tokens'


		def get_event_id(event: str, itask: 'TaskProxy') -> str:

events: refactor key and make the event message available to handlers #5769

events: refactor key and make the event message available to handlers #5769

Conversation

oliver-sanders commented Oct 12, 2023 • edited Loading

oliver-sanders Oct 18, 2023

Choose a reason for hiding this comment

wxtim Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

oliver-sanders commented Oct 23, 2023

dwsutherland left a comment • edited Loading

Choose a reason for hiding this comment

dwsutherland Nov 22, 2023

Choose a reason for hiding this comment

oliver-sanders Nov 22, 2023

Choose a reason for hiding this comment

oliver-sanders Jan 3, 2024

Choose a reason for hiding this comment

oliver-sanders Jan 3, 2024

Choose a reason for hiding this comment

oliver-sanders Jan 3, 2024

Choose a reason for hiding this comment

wxtim Jan 4, 2024

Choose a reason for hiding this comment

wxtim commented Jan 4, 2024 • edited Loading

hjoliver commented Jan 16, 2024

hjoliver left a comment

Choose a reason for hiding this comment

wxtim commented Jan 16, 2024

oliver-sanders commented Jan 16, 2024 • edited Loading

wxtim left a comment • edited Loading

Choose a reason for hiding this comment

oliver-sanders commented Jan 17, 2024 • edited Loading

oliver-sanders commented Oct 12, 2023 •

edited

Loading

wxtim Nov 14, 2023 •

edited

Loading

dwsutherland left a comment •

edited

Loading

wxtim commented Jan 4, 2024 •

edited

Loading

oliver-sanders commented Jan 16, 2024 •

edited

Loading

wxtim left a comment •

edited

Loading

oliver-sanders commented Jan 17, 2024 •

edited

Loading