-
Notifications
You must be signed in to change notification settings - Fork 94
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
Restore default for PBS max job name len. #5386
Conversation
e00c962
to
7b300b4
Compare
job_name_len_max = job_conf['platform'].get( | ||
"job name length maximum", | ||
self.JOB_NAME_LEN_MAX | ||
) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :(
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
job_name_len_max = job_conf['platform'].get( | ||
"job name length maximum", | ||
self.JOB_NAME_LEN_MAX | ||
) |
There was a problem hiding this comment.
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)
job_name_len_max = job_conf['platform'].get( | ||
"job name length maximum", | ||
self.JOB_NAME_LEN_MAX | ||
) |
There was a problem hiding this comment.
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.
This needs to be used if it is not set in the site config and was mistakenly removed.
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
0d1c169
to
3399adb
Compare
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
This is a pull request describing the issue to be fixed.
Issue
If
global.cylc[platforms][<platform>]job name length maximum
is unsetcylc.flow.job_runner_handlers.pbs.PBSHandler.format_directives
will fail with a key error.Instead it should fall back to the value contained in
JOB_NAME_LEN_MAX
.Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).CHANGES.md
entry included if this is a change that can affect users?.?.x
branch.