-
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
Param values under runtime #2435
Conversation
(Failing PEP8 test on Travis CI.) |
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.
Some quick comments.
lib/cylc/param_expand.py
Outdated
if msg is not None: | ||
raise ParamExpandError("ERROR, parameter %s not supported" | ||
" here: %s" % (msg, origin)) | ||
used_params = [i.strip() for i in p_tmpl[1:-1].split(',')] |
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 wonder if it is worth adjusting what REC_P_NAME
captures so we don't have to do p_tmpl[1:-1]
. (REC_P_NAME
is used in another location with a similar logic.)
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 think the code will be easier to understand if we have something like captured_params
and used_params
.
(I don't understand why the failed test is failing on Travis CI. It is as if you have already merged master or #2431 into this branch.) |
(In which case, you should have a simple block to convert the regex-captured integer string into an |
9a25fe1
to
82da39f
Compare
82da39f
to
35efc40
Compare
f3ae6ad
to
f5f4952
Compare
(yeah that was weird - something was returning an int on Travis CI and a string in my environment. I've rebased to master now).
|
ba2206c
to
38dc0bd
Compare
@oliver-sanders please sanity check. |
I find it a little confusing that the following two examples work and are equivalent: [runtime]
[[task<param=value>]]
inherit = FAM<param> [runtime]
[[task<param=value>]]
inherit = FAM<param=value> Whereas these examples doesn't work: [runtime]
[[task<param>]]
inherit = FAM<param=value> [runtime]
[[foo]]
inherit = FAM<param=value> |
@oliver-sanders - your first two examples make perfect sense to me, given that what gets expanded/replicated across parameter ranges is task names in the graph and corresponding task definition sections under runtime ... settings underneath a runtime namespace do not get expanded independently, they need to take the value of the parameter in the heading (either implicitly - as before this PR - or explicitly - as on this PR). I'm trying to figure out if your 3rd and 4th examples makes sense: (3) is it plausible to have all instances of a parameterized task inherit from a single instance of a family that is expanded over the same parameter? and (4) is there a case for a single unparameterized task that inherits from a specific instance of a parameterized family? Maybe we should support those in principle even if they seem unlikely in practice. (4) suggests this though, which is definitely nonsensical: [runtime]
[[foo]]
inherit = FAM<param> |
Probably not, though once we set the president that
Maybe? [cylc]
[[parameters]]
transport = car, train, plane
[scheduling]
[[dependencies]]
graph = """
start => travel<transport>
airport_security => travel<transport=plane>
"""
[runtime]
[start]
[TRANSPORT<transport>]
script = # mv
[TRANSPORT<transport=plane>]
[[[environment]]]
FLIGHT_NUMBER=abc001
TERMINAL=3
[travel<transport>]
inherit = travel<transport>
[airport_security]
inherit = TRANSPORT<transport=plane> |
Coming back to this we could just put in an error message to cover the latter two cases. The stock error is somewhat misleading:
|
@oliver-sanders - based on your example (which is mildly convincing!) I've decided to allow inheritance from any specific parameterized parent. The value of a parameter in the parent name can either be explicit ( |
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.
Looks Good!
Supersedes #2427
Close #2414