Open
Conversation
shiklomanov-an
requested changes
May 7, 2026
Collaborator
shiklomanov-an
left a comment
There was a problem hiding this comment.
The variable = "unset" thing is an anti-pattern, and cuts off the ability to set this directly via environment variable (which seems useful to preserve). But otherwise, this looks good — happy to approve once that's resolved.
Comment on lines
+56
to
+59
| if self.cylc_timeout is not None: | ||
| os.environ['SWELL_CYLC_TIMEOUT'] = self.cylc_timeout | ||
| else: | ||
| os.environ['SWELL_CYLC_TIMEOUT'] = 'unset' |
Collaborator
There was a problem hiding this comment.
Two things:
- Per my comment above, setting the variable to
"unset"is an anti-pattern. I would just leave the variable literally unset.
if self.cylc_timeout is not None:
os.environ['SWELL_CYLC_TIMEOUT'] = self.cylc_timeout
# no else- Do you want to give users the ability to control this directly via environment variable? Might come in handy for certain edge cases. If so, consider leaving any user-specified values of
SELF_CYLC_TIMEOUTunchanged.
if self.cylc_timeout and 'SELF_CYLC_TIMEOUT' not in os.environ:
Collaborator
Author
There was a problem hiding this comment.
Fixed, thanks. As for 2, the way it is currently a user would have to go out of their way to specify -t, so I think it would be best to allow that to override the environment variable, so the hierarchy of control would be (in ascending order)
- Cylc default
- Value in global.cylc
- User-set environment variable
- User-specified value on command line
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@rtodling This pr adds the flag '-t` to the swell launch command to dynamically set the cylc stall timeout.
swell launch <experiment> -b -t PT30SThe launch timeout is usually set to an hour by default, or to the value under
~/.cylc/flow/global.cylc. This allows the user to set it manually, intended mostly for the purpose of throwing an error immediately after the suite fails. The implementation here is not ideal, but this will be made simpler in #656.