From fef13fbbded4bdc94e8903cc7ac1b07dd9e88a98 Mon Sep 17 00:00:00 2001 From: Tim Pillinger Date: Fri, 10 Jan 2025 15:32:59 +0000 Subject: [PATCH 1/6] -W should only be highlighted by Cylc lint when being used in a way which indicates use of LSF. --- changes.d/6551.fix.md | 1 + cylc/flow/scripts/lint.py | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-) create mode 100644 changes.d/6551.fix.md diff --git a/changes.d/6551.fix.md b/changes.d/6551.fix.md new file mode 100644 index 00000000000..8db17899f0a --- /dev/null +++ b/changes.d/6551.fix.md @@ -0,0 +1 @@ +Lint: Fix bug where lint warned about use of legitimate -W directive for PBS. \ No newline at end of file diff --git a/cylc/flow/scripts/lint.py b/cylc/flow/scripts/lint.py index 88f9a0ed5b8..aaa33921ca9 100755 --- a/cylc/flow/scripts/lint.py +++ b/cylc/flow/scripts/lint.py @@ -189,8 +189,12 @@ def get_wallclock_directives(): 'TIME_LIMIT_DIRECTIVE', None ) - if directive: - directives[module.name] = directive + if directive and directive == '-W': + # LSF directive -W needs to have a particular form to + # avoid matching PBS directive -W: + directives[module.name] = re.compile(r'^-W\s+(\d+:)?\d+$') + elif directive: + directives[module.name] = re.compile(rf'^{directive}') return directives @@ -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 + False + >>> this(' -W 422') + {'directive': '-W 422'} + >>> this(' -W foo=42') # Legit PBS use case. + False + >>> this(' -W foo="Hello World"') # Legit PBS use case. + False """ for directive in set(WALLCLOCK_DIRECTIVES.values()): - if line.strip().startswith(directive): + if directive.findall(line.strip()): return {'directive': line.strip()} return False From bcc9ed03d6e9e495d34359a381c4b612beae18da Mon Sep 17 00:00:00 2001 From: Tim Pillinger Date: Fri, 10 Jan 2025 15:55:10 +0000 Subject: [PATCH 2/6] demonstrate the any old pbs -l wallclock value will be picked up --- cylc/flow/scripts/lint.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cylc/flow/scripts/lint.py b/cylc/flow/scripts/lint.py index aaa33921ca9..0832d8d2730 100755 --- a/cylc/flow/scripts/lint.py +++ b/cylc/flow/scripts/lint.py @@ -194,7 +194,7 @@ def get_wallclock_directives(): # avoid matching PBS directive -W: directives[module.name] = re.compile(r'^-W\s+(\d+:)?\d+$') elif directive: - directives[module.name] = re.compile(rf'^{directive}') + directives[module.name] = re.compile(rf'^{directive}.*') return directives @@ -221,6 +221,8 @@ def check_wallclock_directives(line: str) -> Union[Dict[str, str], bool]: False >>> this(' -W foo="Hello World"') # Legit PBS use case. False + >>> this(' -l walltime whatever') + {'directive': '-l walltime whatever'} """ for directive in set(WALLCLOCK_DIRECTIVES.values()): if directive.findall(line.strip()): From b902f4eb6d86e95e02c7bde1a23457fbf0da6775 Mon Sep 17 00:00:00 2001 From: Tim Pillinger Date: Fri, 10 Jan 2025 16:07:16 +0000 Subject: [PATCH 3/6] fix tests --- cylc/flow/scripts/lint.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cylc/flow/scripts/lint.py b/cylc/flow/scripts/lint.py index 0832d8d2730..c429ec4d693 100755 --- a/cylc/flow/scripts/lint.py +++ b/cylc/flow/scripts/lint.py @@ -201,7 +201,7 @@ def get_wallclock_directives(): WALLCLOCK_DIRECTIVES = get_wallclock_directives() -def check_wallclock_directives(line: str) -> Union[Dict[str, str], bool]: +def check_wallclock_directives(line: str) -> Dict[str, str]: """Check for job runner specific directives equivalent to exection time limit. @@ -214,20 +214,20 @@ def check_wallclock_directives(line: str) -> Union[Dict[str, str], bool]: >>> this(' -W 42:22') {'directive': '-W 42:22'} >>> this(' -W 42:22/hostname') # Legit LSF use case - False + {} >>> this(' -W 422') {'directive': '-W 422'} >>> this(' -W foo=42') # Legit PBS use case. - False + {} >>> this(' -W foo="Hello World"') # Legit PBS use case. - False + {} >>> this(' -l walltime whatever') {'directive': '-l walltime whatever'} """ for directive in set(WALLCLOCK_DIRECTIVES.values()): if directive.findall(line.strip()): return {'directive': line.strip()} - return False + return {} def check_jinja2_no_shebang( From 902fcd26c983fd1c4d6e2c656309a1503e8f762f Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 14 Jan 2025 13:03:26 +0000 Subject: [PATCH 4/6] Update cylc/flow/scripts/lint.py --- cylc/flow/scripts/lint.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cylc/flow/scripts/lint.py b/cylc/flow/scripts/lint.py index c429ec4d693..ddefb039034 100755 --- a/cylc/flow/scripts/lint.py +++ b/cylc/flow/scripts/lint.py @@ -192,7 +192,7 @@ def get_wallclock_directives(): if directive and directive == '-W': # LSF directive -W needs to have a particular form to # avoid matching PBS directive -W: - directives[module.name] = re.compile(r'^-W\s+(\d+:)?\d+$') + directives[module.name] = re.compile(r'^-W\s*=?\s*(\d+:)?\d+$') elif directive: directives[module.name] = re.compile(rf'^{directive}.*') return directives From 16a55bdb12866cbaafd2b9d605014833a329be11 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 14 Jan 2025 13:04:48 +0000 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- changes.d/6551.fix.md | 2 +- cylc/flow/scripts/lint.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/changes.d/6551.fix.md b/changes.d/6551.fix.md index 8db17899f0a..3f2c6c0638f 100644 --- a/changes.d/6551.fix.md +++ b/changes.d/6551.fix.md @@ -1 +1 @@ -Lint: Fix bug where lint warned about use of legitimate -W directive for PBS. \ No newline at end of file +Fix bug in `cylc lint` S014 where it warned about use of legitimate `-W` directive for PBS. \ No newline at end of file diff --git a/cylc/flow/scripts/lint.py b/cylc/flow/scripts/lint.py index ddefb039034..1187b9b1ae1 100755 --- a/cylc/flow/scripts/lint.py +++ b/cylc/flow/scripts/lint.py @@ -192,7 +192,7 @@ def get_wallclock_directives(): if directive and directive == '-W': # LSF directive -W needs to have a particular form to # avoid matching PBS directive -W: - directives[module.name] = re.compile(r'^-W\s*=?\s*(\d+:)?\d+$') + directives[module.name] = re.compile(r'^-W\s*=?\s*(\d+:)?\d+[^/]*$') elif directive: directives[module.name] = re.compile(rf'^{directive}.*') return directives From 9470bddb6025b9b4cc32c08062344ee04c243386 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Wed, 15 Jan 2025 07:16:30 +0000 Subject: [PATCH 6/6] Update cylc/flow/scripts/lint.py --- cylc/flow/scripts/lint.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cylc/flow/scripts/lint.py b/cylc/flow/scripts/lint.py index 1187b9b1ae1..a78686130ff 100755 --- a/cylc/flow/scripts/lint.py +++ b/cylc/flow/scripts/lint.py @@ -192,7 +192,8 @@ def get_wallclock_directives(): if directive and directive == '-W': # LSF directive -W needs to have a particular form to # avoid matching PBS directive -W: - directives[module.name] = re.compile(r'^-W\s*=?\s*(\d+:)?\d+[^/]*$') + directives[module.name] = re.compile( + r'^-W\s*=?\s*(\d+:)?\d+[^/]*$') elif directive: directives[module.name] = re.compile(rf'^{directive}.*') return directives