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

backport fix from #5712 #5927

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes.d/fix.5712.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed a bug preventing simulation of execution retries when `submission retry delays` is not set.
6 changes: 3 additions & 3 deletions cylc/flow/task_events_mgr.py
Original file line number Diff line number Diff line change
Expand Up @@ -803,13 +803,13 @@
f"[{itask}] "
f"{self.FLAG_RECEIVED_IGNORED}{message}{timestamp}"
)

else:
return False
Copy link
Member Author

Choose a reason for hiding this comment

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

@MetRonnie This isn't the same as the change on master - It's the response to this comment: #5927 (comment). I wanted to see what CI made of it. This is the commit where I changed it from what's on master.

elif flag == self.FLAG_POLLED_IGNORED:
LOG.warning(
f"[{itask}] "
f"{self.FLAG_POLLED_IGNORED}{message}{timestamp}"
)
return False
return False

Check warning on line 812 in cylc/flow/task_events_mgr.py

View check run for this annotation

Codecov / codecov/patch

cylc/flow/task_events_mgr.py#L812

Added line #L812 was not covered by tests

severity = cast(int, LOG_LEVELS.get(severity, INFO))
# Demote log level to DEBUG if this is a message that duplicates what
Expand Down
4 changes: 2 additions & 2 deletions cylc/flow/task_job_mgr.py
Original file line number Diff line number Diff line change
Expand Up @@ -979,8 +979,8 @@ def _set_retry_timers(
rtconfig = itask.tdef.rtconfig

submit_delays = (
rtconfig['submission retry delays']
or itask.platform['submission retry delays']
rtconfig.get('submission retry delays', [])
Copy link
Member Author

@wxtim wxtim Jan 17, 2024

Choose a reason for hiding this comment

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

The absence of this may make the test workflow fail, but it can depend on whats in your global cylc. Have a look at the artifacts in https://github.com/cylc/cylc-flow/actions/runs/7556705094/job/20574317445?pr=5927

Copy link
Member

@MetRonnie MetRonnie Jan 22, 2024

Choose a reason for hiding this comment

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

This fix (which you are saying is for bug 2), does not appear to be on master / #5712

Copy link
Member Author

@wxtim wxtim Jan 23, 2024

Choose a reason for hiding this comment

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

You are right that it is not on master - I thought it had gone in with #5712, but it hasn't.

However, I'm claiming this is the fix to bug1: You have to fix bug 1 to see bug 2.

or itask.platform.get('submission retry delays', [])
)

for key, delays in [
Expand Down
32 changes: 32 additions & 0 deletions tests/functional/modes/03-simulation.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#!/usr/bin/env bash
# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.

# Test that simulation mode runs, and reruns a failed task successfully
# when execution retry delays is configured.

. "$(dirname "$0")/test_header"
set_test_number 2

install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}"
run_ok "${TEST_NAME_BASE}-validate" cylc validate "${WORKFLOW_NAME}"
workflow_run_ok "${TEST_NAME_BASE}-run" \
cylc play \
--no-detach \
--mode=simulation \
--reference-test "${WORKFLOW_NAME}"
purge
exit
16 changes: 16 additions & 0 deletions tests/functional/modes/03-simulation/flow.cylc
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
[scheduler]
[[events]]
workflow timeout = PT30S

[scheduling]
initial cycle point = 2359
[[graph]]
R1 = get_observations

[runtime]
[[get_observations]]
execution retry delays = PT2S
[[[simulation]]]
Comment on lines +12 to +13
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
execution retry delays = PT2S
[[[simulation]]]
execution retry delays = PT1S
[[[simulation]]]
default run length = PT0S

Copy link
Member Author

Choose a reason for hiding this comment

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

The point of this test was, at least partly to test the interaction of this setting.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, what do you mean? I'm just suggesting this to speed up the test

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want the default run length.

fail cycle points = all
fail try 1 only = True

2 changes: 2 additions & 0 deletions tests/functional/modes/03-simulation/reference.log
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
23590101T0000Z/get_observations -triggered off [] in flow 1
23590101T0000Z/get_observations -triggered off [] in flow 1
Loading