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

Xtrigger sequence fix #3285

Merged
merged 10 commits into from Aug 15, 2019

Conversation

@hjoliver
Copy link
Contributor

commented Aug 12, 2019

These changes partially address #3283 (companion PR to master will complete).

Major:

  • record xtrigger functions against cycling sequence, and only use those valid for the sequence when task proxies are instantiated.

Minor:

  • better unifies the handling of clock- and general-xtriggers
  • wall_clock works with positional or keyword offset arg (another problem identified by @trwhitcomb)
  • disallows clock triggers with integer cycling

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).
  • Appropriate tests are included (functional).
  • Appropriate change log entry included.
  • No documentation update required (this just makes xtriggers work as they should).

@hjoliver hjoliver force-pushed the hjoliver:xtrigger-sequence-fix branch 2 times, most recently from 1727f93 to 128c84c Aug 12, 2019

@hjoliver hjoliver self-assigned this Aug 12, 2019

@hjoliver hjoliver added the bug label Aug 12, 2019

@hjoliver hjoliver added this to the next-release milestone Aug 12, 2019

@cylc cylc deleted a comment from codacy-bot Aug 12, 2019

@cylc cylc deleted a comment from codacy-bot Aug 12, 2019

@hjoliver hjoliver force-pushed the hjoliver:xtrigger-sequence-fix branch from ac23fc1 to 5bbdf57 Aug 12, 2019

@hjoliver hjoliver marked this pull request as ready for review Aug 12, 2019

@hjoliver hjoliver requested a review from matthewrmshin Aug 12, 2019

@cylc cylc deleted a comment from codacy-bot Aug 12, 2019

@hjoliver hjoliver referenced this pull request Aug 12, 2019
6 of 6 tasks complete
@matthewrmshin
Copy link
Member

left a comment

Generally happy with these changes. A few comments on style.

lib/cylc/task_pool.py Show resolved Hide resolved
lib/cylc/config.py Show resolved Hide resolved
lib/cylc/xtrigger_mgr.py Outdated Show resolved Hide resolved
tests/xtriggers/03-sequence.t Show resolved Hide resolved
tests/xtriggers/03-sequence.t Outdated Show resolved Hide resolved

@cylc cylc deleted a comment from codacy-bot Aug 13, 2019

@hjoliver hjoliver force-pushed the hjoliver:xtrigger-sequence-fix branch from c518a21 to 1c95277 Aug 13, 2019

@cylc cylc deleted a comment from codacy-bot Aug 13, 2019

@cylc cylc deleted a comment from codacy-bot Aug 13, 2019

@hjoliver hjoliver force-pushed the hjoliver:xtrigger-sequence-fix branch from 1778f34 to 63bed59 Aug 14, 2019

@cylc cylc deleted a comment from codacy-bot Aug 14, 2019

@hjoliver

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

(rebased)

@hjoliver hjoliver force-pushed the hjoliver:xtrigger-sequence-fix branch from 63bed59 to 18aa1ea Aug 15, 2019

@cylc cylc deleted a comment from codacy-bot Aug 15, 2019

@codacy-bot

This comment has been minimized.

Copy link

commented Aug 15, 2019

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- lib/cylc/xtrigger_mgr.py  2
         

See the complete overview on Codacy

@cylc cylc deleted a comment from codacy-bot Aug 15, 2019

@dwsutherland
Copy link
Contributor

left a comment

LGTM 👍

Ran it on my system, and the xtriggers were on sequence.

(just Travis now)

@matthewrmshin matthewrmshin merged commit 7ecbf99 into cylc:7.8.x Aug 15, 2019

4 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 86.84% of diff hit (target 58%)
Details
codecov/project 84.28% (target 58%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hjoliver hjoliver deleted the hjoliver:xtrigger-sequence-fix branch Aug 15, 2019

@hjoliver

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Thanks @dwsutherland 👍

@hjoliver

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Yay, the 7.8.x test battery passed, unbelievable 😮 (I stole a few fixes from @matthewrmshin's work on master).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.