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.

Sign up for GitHub

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

Jump to bottom

Restore default for PBS max job name len. #5386

Merged
merged 7 commits into from
Mar 8, 2023
Merged

Restore default for PBS max job name len. #5386

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ffaa102
Restore default for PBS max job name len.
wxtim Mar 1, 2023
bf7dca8
Update tests/unit/job_runner_handlers/test_pbs.py
wxtim Mar 2, 2023
616c57f
response to review
wxtim Mar 2, 2023
37fde34
make names of tests safer
wxtim Mar 2, 2023
3399adb
fixed conflict in changes
wxtim Mar 2, 2023
3781eb0
Update tests/unit/job_runner_handlers/test_pbs.py
wxtim Mar 2, 2023
f334937
fix test
wxtim Mar 2, 2023
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
4 changes: 4 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ ones in. -->
[#5384](https://github.com/cylc/cylc-flow/pull/5384) -
Fixes `cylc set-verbosity`.

[#5386](https://github.com/cylc/cylc-flow/pull/5386) Fix bug where
absence of `job name length maximum` in PBS platform settings would cause
Cylc to crash when preparing the job script.

wxtim marked this conversation as resolved.
Show resolved Hide resolved
-------------------------------------------------------------------------------
## __cylc-8.1.2 (<span actions:bind='release-date'>Released 2023-02-20</span>)__

Expand Down
5 changes: 4 additions & 1 deletion cylc/flow/job_runner_handlers/pbs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ def format_directives(self, job_conf):
f'{tokens["task"]}.{tokens["cycle"]}'
f".{job_conf['workflow_name'].replace('/', '-')}"
)
job_name_len_max = job_conf['platform']["job name length maximum"]
job_name_len_max = job_conf['platform'].get(
"job name length maximum",
self.JOB_NAME_LEN_MAX
)
Comment on lines +99 to +102
Copy link
Member

Choose a reason for hiding this comment

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

Thinking back, this wasn't working around the problem with OrderedDictWithDefaults not working right with .get was it?

Copy link
Member

@MetRonnie MetRonnie Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is a revert of f12a089. Would like to know why that commit did it this way in the first place. (Edit: only a partial revert actually)

Copy link
Member

Choose a reason for hiding this comment

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

There isn't any information on that.

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 can't remember that problem. :(

Copy link
Member

Choose a reason for hiding this comment

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

Judging from my description of the OrderedDictWithDefaults.get() problem here: #4975 (comment), I think this is fine.

Copy link
Member

Choose a reason for hiding this comment

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

Can someone test this out with and without config defaults, for localhost and something-else, just to be safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can someone test this out with and without config defaults, for localhost and something-else, just to be safe.

How do I test it out with localhost - it needs a pbs platform?

Copy link
Member

Choose a reason for hiding this comment

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

You can take the platform.get code snippet out and put it anywhere you like, no need to test in-situ.

Copy link
Member

@oliver-sanders oliver-sanders Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

$ cylc config -i '[platforms][localhost]'
ssh command = ssh -oBatchMode=yes -oConnectTimeout=8 -oStrictHostKeyChecking=no
copyable environment variables = FCM_VERSION
submission polling intervals = PT1H
execution polling intervals = PT1H
clean job submission environment = True
diff --git a/cylc/flow/cfgspec/globalcfg.py b/cylc/flow/cfgspec/globalcfg.py
index 17ca5d98d..dbf2baed1 100644
--- a/cylc/flow/cfgspec/globalcfg.py
+++ b/cylc/flow/cfgspec/globalcfg.py
@@ -1891,6 +1891,8 @@ class GlobalConfig(ParsecConfig):
         # Flesh out with defaults
         self.expand()
 
+        breakpoint()
+
         self._no_platform_group_name_overlap()
         with suppress(KeyError):
             validate_platforms(self.sparse['platforms'])
(Pdb) self.get(['platforms', 'localhost']).get('job runner')  # default
'background'
(Pdb) self.get(['platforms', 'localhost']).get('execution polling intervals')  # manually specified
[3600.0]

.get appears to work fine for both, just needed to confirm that especially as we know there's strange behaviour with these objects.

if job_name_len_max:
directives["-N"] = directives["-N"][0:job_name_len_max]

Expand Down
31 changes: 28 additions & 3 deletions tests/unit/job_runner_handlers/test_pbs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@
from cylc.flow.job_runner_handlers.pbs import JOB_RUNNER_HANDLER


VERY_LONG_STR = 'x' * 240


@pytest.mark.parametrize(
'job_conf,lines',
[
( # basic
pytest.param(
{
'directives': {},
'execution_time_limit': 180,
Expand All @@ -40,8 +43,28 @@
'#PBS -e cylc-run/chop/log/job/1/axe/01/job.err',
'#PBS -l walltime=180',
],
id='basic'
),
pytest.param(
{
'directives': {},
'execution_time_limit': 180,
'job_file_path': 'cylc-run/chop/log/job/1/axe/01/job',
'workflow_name': 'chop',
'task_id': VERY_LONG_STR,
'platform': {
'job runner': 'pbs',
}
},
[
f'#PBS -N None.{VERY_LONG_STR[:231]}',
wxtim marked this conversation as resolved.
Show resolved Hide resolved
'#PBS -o cylc-run/chop/log/job/1/axe/01/job.out',
'#PBS -e cylc-run/chop/log/job/1/axe/01/job.err',
'#PBS -l walltime=180',
],
id='long-job-name'
),
( # super short job name length maximum
pytest.param(
{
'directives': {},
'execution_time_limit': 180,
Expand All @@ -59,8 +82,9 @@
'#PBS -e cylc-run/chop/log/job/1/axe/01/job.err',
'#PBS -l walltime=180',
],
id='truncate-job-name'
),
( # some useful directives
pytest.param(
{
'directives': {
'-q': 'forever',
Expand All @@ -85,6 +109,7 @@
'#PBS -V',
'#PBS -l mem=256gb',
],
id='custom-directives'
),
],
)
Expand Down