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

Lint: Only warn about -W when used for LSF #6551

Open
wants to merge 3 commits into
base: 8.4.x
Choose a base branch
from

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jan 10, 2025

Closes #6550

-W should only be highlighted by Cylc lint when being used in a way which indicates use of LSF.

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).
  • Changelog entry included if this is a change that can affect users
  • Very small, self documenting.
  • Raised against 8.4.x, bugfix

@wxtim wxtim self-assigned this Jan 10, 2025
@wxtim wxtim force-pushed the lint.disambiguate_-W branch from 575053f to 404adc1 Compare January 10, 2025 15:36
cylc/flow/scripts/lint.py Outdated Show resolved Hide resolved
@wxtim wxtim force-pushed the lint.disambiguate_-W branch from 316ded6 to fef13fb Compare January 10, 2025 15:41
@@ -209,9 +213,17 @@ def check_wallclock_directives(line: str) -> Union[Dict[str, str], bool]:
>>> this = check_wallclock_directives
>>> this(' -W 42:22')
{'directive': '-W 42:22'}
>>> this(' -W 42:22/hostname') # Legit LSF use case
Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at https://www.ibm.com/docs/en/spectrum-lsf/10.1.0?topic=o-w-1, I think that this is an LSF wallclock directive which can't be handled by execution time limit and therefore is a legitimate case for using -W with LSF.

Copy link
Member

Choose a reason for hiding this comment

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

Can you do multiple things with this opt?

e.g -W 00:02 foo=bar

Copy link
Member Author

Choose a reason for hiding this comment

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

In LSF or PBS?

In LSF, not really.

In PBS, not really - only key=value pairs.

I don't think that the two are mistakeable.

@wxtim wxtim modified the milestones: 8.5.0, 8.4.1 Jan 10, 2025
@wxtim wxtim added bug Something is wrong :( small labels Jan 10, 2025
@oliver-sanders
Copy link
Member

I found some suggestion in the user guide that the time can take a longer format:

Resource_List.walltime = 00:30:00

#PBS -l walltime=1:00:00

qsub -l ncpus=16 -l walltime=4:00:00 mysubrun

Maybe: (\d{2}:)+.

Might be worth quickly running this rule over avaliable cylc-run dirs to make sure there aren't any false positives.

@wxtim
Copy link
Member Author

wxtim commented Jan 10, 2025

I found some suggestion in the user guide that the time can take a longer format:

This regex is only supposed to match LSF format wallclock strings, avoiding PBS -W which look superficially similar.

The PBS check one will match any case where -l wallclock is present.

I've added an extra test to demonstrate my point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants