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

cylc lint S014: job runner specific timeout #6137

Merged
merged 4 commits into from
Jul 10, 2024

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jun 13, 2024

Closes #6131

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.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim self-assigned this Jun 13, 2024
@wxtim wxtim force-pushed the lint.job_runner_specific_timeout branch from 579c2d2 to d24616b Compare June 13, 2024 15:49
@wxtim wxtim requested review from hjoliver and MetRonnie June 13, 2024 15:49
@wxtim wxtim added the small label Jun 13, 2024
@wxtim wxtim added this to the 8.3.1 milestone Jun 13, 2024
@wxtim wxtim force-pushed the lint.job_runner_specific_timeout branch from d24616b to 700eeec Compare June 19, 2024 08:52
@wxtim wxtim changed the base branch from master to 8.3.x June 19, 2024 08:54
@wxtim wxtim force-pushed the lint.job_runner_specific_timeout branch from 700eeec to 86d24c3 Compare June 19, 2024 08:57
@wxtim wxtim requested review from markgrahamdawson and removed request for hjoliver June 19, 2024 08:59
MetRonnie

This comment was marked as resolved.

@MetRonnie
Copy link
Member

MetRonnie commented Jun 20, 2024

Marked my review comment as resolved but then it's hidden all the comments on the files, which are not resolved. Now I can't change the review comment to unresolved 😑

Please unhide the review and look at the comments

@MetRonnie
Copy link
Member

Also should remove any instances of time limit directives from the docs? E.g.

@MetRonnie MetRonnie modified the milestones: 8.3.1, 8.4.0 Jun 20, 2024
@MetRonnie MetRonnie changed the base branch from 8.3.x to master June 20, 2024 11:28
@wxtim wxtim requested a review from MetRonnie June 20, 2024 11:59
@wxtim
Copy link
Member Author

wxtim commented Jun 20, 2024

Also should remove any instances of time limit directives from the docs? E.g.

This appears to only be an issue for SGE where a not below tells you not to do precisely this!

n.b. Some of these directives still appear in documentation where examples of the job file show how the execution time limit will be converted to directives.

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

The only thing it doesn't pick up on is when multiple directives are specified on the same setting, e.g.

[[[directives]]]
    -l = walltime=60:nodes=1

But otherwise I have tested and am happy with this

@wxtim
Copy link
Member Author

wxtim commented Jun 21, 2024

The only thing it doesn't pick up on is when multiple directives are specified on the same setting, e.g.

[[[directives]]]
    -l = walltime=60:nodes=1

But otherwise I have tested and am happy with this

Not a pattern we see too often

@wxtim wxtim requested a review from markgrahamdawson July 10, 2024 08:07
@markgrahamdawson markgrahamdawson merged commit e28e0c0 into cylc:master Jul 10, 2024
26 of 27 checks passed
@wxtim wxtim deleted the lint.job_runner_specific_timeout branch July 10, 2024 09:02
@MetRonnie MetRonnie changed the title Lint.job runner specific timeout cylc lint S014: job runner specific timeout Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint: Rule-Request: Avoid Job Runner Wallclock Directives
3 participants