-
Notifications
You must be signed in to change notification settings - Fork 95
Fix platform subshell evaluating more than once for tasks triggered in paused workflow #7035
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
base: 8.6.x
Are you sure you want to change the base?
Conversation
| async def test_trigger_whilst_paused_preparing(flow, scheduler, run, complete): | ||
| """It should run "preparing" tasks even if the workflow is paused. | ||
| Remote init leaves tasks as preparing for a while. These must still be | ||
| pushed through to running, even if triggered while the workflow is paused. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test added back in d3f5f68 since it was changed on 8.6.x
| # Both of these cases should validate ok. | ||
| run_ok "${TEST_NAME_BASE}-validate" \ | ||
| cylc validate "${WORKFLOW_NAME}" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should validate ok! This is detectable at config parse time, however we are unlikely to fix this now as the deprecated syntax is to be removed in the next minor release
4aea66b to
c2d5048
Compare
c2d5048 to
dfcdcab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @MetRonnie - but I'll leave you to check the conflict with #7054 (huh, this PR is older than that one!)
2564b21 to
d3f5f68
Compare
| pre_prep_tasks.update({ | ||
| itask | ||
| for itask in self.pool.get_tasks() | ||
| if itask.state(TASK_STATUS_PREPARING) | ||
| } | ||
| and itask.waiting_on_job_prep | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliver-sanders This PR pre-dated #7054 and had basically the same fix.
However #7054 missed L1376 - I have consolidated the two in this PR.
One difference here is that I did not delete the check for itask.state(TASK_STATUS_PREPARING), I added the check for itask.waiting_on_job_prep in addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK now waiting on @oliver-sanders to check that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any need to have itask.state(TASK_STATUS_PREPARING) in combination with itask.waiting_on_job_prep, but it can't do any harm.
- All task proxies that are `waiting_on_job_prep` should be in the preparing state, so remove redundant check - Add back in simple trigger-while-paused integration test
d3f5f68 to
a3ddfc5
Compare
Closes #6994
Stop trying to submit the triggered tasks once
itask.waiting_on_job_prep == False.This is kind of a follow-up to #6836, #6768, #6345, #5062. It also predates and fixes the same bug as #7054.
Also fixes a scheduler TypeError crash if platform subshell evaluates to an empty string.
Also fixes an incomplete warning message
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.?.?.xbranch.