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.

Sign up for GitHub

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

Jump to bottom

Integer flow labels with flow metadata. #4300

Merged
merged 14 commits into from
Oct 20, 2021
Merged

Integer flow labels with flow metadata. #4300

merged 14 commits into from
Oct 20, 2021

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jul 11, 2021 •
edited
Loading

Supersede #4287 (post side-meeting discussion)
Close #3744

REBASED ON TOP OF #4343 - will un-draft this when that's merged

  • the original flow is labeled with integer flow number 1
  • the flow number increments as new flows are triggered
  • task proxies carry a set of flow numbers representing all of the flows they belong to
    • gets updated on flow mergers (which happens one task at a time)
  • task proxies pass their flow numbers to spawned graph-children
  • cylc stop --flow=INT stops a flow by removing the INT label from all task proxies that have it
    • if a task proxy has no flow numbers left, it does not spawn children
  • minimal flow metadata:
    • timestamp
    • optional description string
  • improved task logging (partially address Cleaner log messages. #3647)

Example showing flow number and task state-change logging:

shot

shot2

Requirements 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.py and conda-environment.yml.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.

@hjoliver
Copy link
Member Author

hjoliver commented Jul 11, 2021 •
edited
Loading

TODO transfer ideas for expanded flow metadata to another issue #4287 (comment)
See #4412

@hjoliver hjoliver mentioned this pull request Jul 11, 2021
Closed
8 tasks
@hjoliver hjoliver added this to the cylc-8.0.0 milestone Jul 19, 2021
@hjoliver hjoliver modified the milestones: cylc-8.0.0, cylc-8.0b3 Aug 4, 2021
@hjoliver hjoliver force-pushed the integer-flow-labels branch 3 times, most recently from 94b155b to 62c4303 Compare August 23, 2021 07:29
@hjoliver hjoliver force-pushed the integer-flow-labels branch 2 times, most recently from f53dbd7 to 0db5464 Compare September 6, 2021 03:09
@hjoliver
Copy link
Member Author

hjoliver commented Sep 6, 2021 •
edited
Loading

REBASED ON TOP OF #4343 - will undraft this when that's merged

@hjoliver hjoliver mentioned this pull request Sep 6, 2021
Open
@hjoliver hjoliver force-pushed the integer-flow-labels branch 4 times, most recently from 8711c3e to d157621 Compare September 15, 2021 04:10
@hjoliver hjoliver marked this pull request as ready for review September 15, 2021 04:10
@hjoliver hjoliver mentioned this pull request Sep 15, 2021
Open
@hjoliver hjoliver force-pushed the integer-flow-labels branch 2 times, most recently from f617770 to 7f29be3 Compare September 22, 2021 02:31
@hjoliver
Copy link
Member Author

rebased

@hjoliver
Copy link
Member Author

hjoliver commented Oct 1, 2021 •
edited
Loading

(This one needs to go in soon; I'll try to deal with the conflicts on the weekend before I leave town rebased).

@hjoliver hjoliver force-pushed the integer-flow-labels branch from 7f29be3 to 64e66ce Compare October 1, 2021 04:39
@hjoliver
Copy link
Member Author

hjoliver commented Oct 1, 2021

@oliver-sanders - plz reassign etc. according to time and availability!

oliver-sanders
oliver-sanders reviewed Oct 4, 2021
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Wow, this one got big. Code looks good, still need to test.

cylc/flow/flow_mgr.py Show resolved Hide resolved
cylc/flow/scheduler.py Outdated Show resolved Hide resolved
cylc/flow/flow_mgr.py Outdated Show resolved Hide resolved
cylc/flow/scripts/set_outputs.py Outdated Show resolved Hide resolved
cylc/flow/task_job_mgr.py Outdated Show resolved Hide resolved
@hjoliver hjoliver force-pushed the integer-flow-labels branch from 64e66ce to 548583f Compare October 13, 2021 05:52
@hjoliver
Copy link
Member Author

(comments addressed, and rebased)

@hjoliver hjoliver force-pushed the integer-flow-labels branch from 81437de to 2e49473 Compare October 14, 2021 02:04
@hjoliver hjoliver force-pushed the integer-flow-labels branch 2 times, most recently from 530c218 to 6262d16 Compare October 14, 2021 04:05
@hjoliver hjoliver force-pushed the integer-flow-labels branch from ee97345 to 4683b26 Compare October 15, 2021 02:57
@hjoliver
Copy link
Member Author

Gnarly rebase after merging the runahead branch 😬

@hjoliver hjoliver force-pushed the integer-flow-labels branch from 4683b26 to f6cace4 Compare October 15, 2021 03:43
@hjoliver hjoliver force-pushed the integer-flow-labels branch from f6cace4 to 07004d7 Compare October 19, 2021 23:47
@hjoliver
Copy link
Member Author

(rebased and deconflicted again)

@datamel datamel self-requested a review October 20, 2021 12:49
datamel
datamel approved these changes Oct 20, 2021
View reviewed changes
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

I have read the code, checked out the branch and run the tests locally. I have not spotted any problems in the logic and workflows are running smoothly for me. Minor notes, a broken f string and a missing arg doc.

The two failures I am getting are also failing for me on master: tests/functional/reload/20-stop-point.t and tests/functional/job-submission/16-timeout.t. I shall have a look into these tomorrow when I have more time.

This change looks good to me, and the logs look tidier which is great. Approved!

cylc/flow/network/resolvers.py Show resolved Hide resolved
cylc/flow/task_pool.py Outdated Show resolved Hide resolved
hjoliver and others added 2 commits October 21, 2021 09:40
@hjoliver @datamel
Update cylc/flow/task_pool.py
0335775 
Co-authored-by: Melanie Hall <37735232+datamel@users.noreply.github.com>
@hjoliver
Address review feedback (tweak some docstrings).
c58eef4
@hjoliver
Copy link
Member Author

Thanks @datamel, applied your suggestions, merging now 🎉

@hjoliver hjoliver merged commit 9f1dce5 into cylc:master Oct 20, 2021
@hjoliver hjoliver deleted the integer-flow-labels branch October 20, 2021 21:57
@hjoliver hjoliver mentioned this pull request Nov 1, 2021
Merged
8 tasks
@MetRonnie MetRonnie added the db change Change to the workflow database structure label Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliver-sanders oliver-sanders oliver-sanders approved these changes

@datamel datamel datamel approved these changes

Assignees

@hjoliver hjoliver

Labels
db change Change to the workflow database structure
Projects
None yet
Milestone
cylc-8.0b3
Development

Successfully merging this pull request may close these issues.

Flow labels and (re)flow metadata
4 participants
@hjoliver @oliver-sanders @datamel @MetRonnie