Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up task output message handling #1761

Merged
merged 9 commits into from
Mar 29, 2016

Conversation

hjoliver
Copy link
Member

Close #1604.

  • Task ID is no longer prepended to output messages at the client end and sent across the network (not necessary as the receiving object in the server has the same ID).
  • Task ID is now only prepended to output messages inside the data structure handed to the broker to match with prerequisites.
  • Treat message outputs the same as standard outputs - TaskID prepended as per the previous bullet point.
  • Deprecated "message offset" placeholders in message trigger strings (see below Clean up task output message handling #1761 (comment))
  • As a consequence of the previous points, simplify output message handling in task_proxy.py by getting rid of a bunch of Task ID prepend/remove operations.
  • Eliminates some annoying redundancy in the suite log: [task-id] ... task-id succeeded is now just [task-id] ...succeeded, etc.
  • Rename some associated modules and classes (e.g. output.py:output to message_output.py:MessageOutput - as this class is only used for message outputs).

@hjoliver hjoliver self-assigned this Mar 14, 2016
@hjoliver hjoliver added this to the soon milestone Mar 14, 2016
@hjoliver
Copy link
Member Author

Re deprecation of cycle point offset placeholders in message trigger strings ... this:

[[[P1M]]]
   graph = foo[-P1M]:out1 => bar
...
[runtime]
   [[foo]]
       script = """
cylc message "files ready for bar at $(cylc cyclepoint --offset=P1M)"
"""
       [[[outputs]]]
             out1 = "output files ready for bar at [P1M]"

is now equivalent to this for all practical purposes:

[[[P1M]]]
   graph = foo[-P1M]:out1 => bar
...
[runtime]
   [[foo]]
       script = cylc message "files ready for bar"
       [[[outputs]]]
             out1 = "files ready for bar"

On master, we enforce the use of the offset placeholders in message output strings in order to distinguish the message strings emitted by successive instances of foo. But this was only necessary because we were not prepending task ID to message strings as is automatically done for the standard outputs (succeeded etc.). Wracking my brains to recall how this crazy situation came about, I believe it goes right back to cylc-2 when there was no dependency graph (and therefore no cycle point offset notation in the graph), so this was the way to implement inter-cycle triggering in the case of message triggers.

I've changed the documentation and examples to remove all references to in-string offset placeholders, but it's still supported in the code for backward compatibility.

else:
offset = get_interval_cls().get_null()
# else: Plain message, no offset.
return offset
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was duplicated in two places.

@hjoliver hjoliver changed the title Clean up use of the task ID prefix in task output messages Clean up task output message handling Mar 15, 2016
@hjoliver hjoliver force-pushed the 1604.no-taskid-in-messages branch from 9932fc5 to fa09fb8 Compare March 15, 2016 01:22
@hjoliver hjoliver force-pushed the 1604.no-taskid-in-messages branch from bc2cbd0 to 20bb930 Compare March 15, 2016 01:39
@hjoliver hjoliver force-pushed the 1604.no-taskid-in-messages branch from 20bb930 to bbf4ccc Compare March 15, 2016 01:43
@hjoliver hjoliver assigned matthewrmshin and unassigned hjoliver Mar 15, 2016
@hjoliver
Copy link
Member Author

@matthewrmshin - please review or re-assign. Test battery passes here.

@matthewrmshin
Copy link
Contributor

./tests/message-triggers/03-new-style.t is failing due to timezone issue.

global warned
global BCOMPAT_MSG_RE_C5
global BCOMPAT_MSG_RE_C6
global DEPRECN_WARN_TMPL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to declare global for constants.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, yeah, just for warned

@matthewrmshin
Copy link
Contributor

Looking good. Some minor issues noted. Test OK (with and without site/user global configuration) in my environment apart from the time zone problem in the new test.

@arjclark please review 2.

@hjoliver
Copy link
Member Author

All comments addressed.

@matthewrmshin
Copy link
Contributor

Tests all good.

@hjoliver hjoliver modified the milestones: next-release, soon Mar 20, 2016
@arjclark
Copy link
Contributor

Looks ok to me. Test battery passing in my environment.

@arjclark arjclark merged commit 7abb89d into cylc:master Mar 29, 2016
@hjoliver hjoliver deleted the 1604.no-taskid-in-messages branch October 18, 2017 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants