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
DM-27633 add a parameters section to Pipelines #159
Conversation
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.
Other than the question and typo, it looks fine to me.
for key, value in new_config.rest.items(): | ||
match = re.match("parameters[.](.*)", value) | ||
if match and match.group(1) in parameters: | ||
new_config.rest[key] = parameters[match.group(1)] |
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.
Should this warn or raise if match and match.group(1) not in parameters
? Or will that fail later in the pipeline construction process?
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 left this open so that if there is a parameter in the pipeline and it matches it is used. But if someone had a value that contained this string that was not a parameter it would allow it to continue in case there was a task somewhere that needed this as a string or something for some other reason. I didn't want to be any more restrictive than I needed to be. Do you think it should be some other way (I definitely am open to other viewpoints here, I just went with what seemed best to me)
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 went back and forth, and I think I've settled on "a warning is good." In the case where a parameter wasn't defined right, it can at least say "No parameter named 'typoe' found". In the case where someone defines a goodIdea: config.parameters.badIdea
, the warning will hopefully annoy them into not using that name.
---------- | ||
instrument : Optional `str` | ||
A string giving the fully qualified path to an instrument object. | ||
If a inherited pipeline defines the same instrument as defined in |
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.
Typo: "an inherited".
Add a parameters section to pipelines that can be used to configure values that appear in multiple config blocks.
Skip the warning associated with inheriting a Pipeline with an instrument defined, if that pipeline is the same instrument as in top most Pipeline.
adb1e3b
to
62d5df9
Compare
7021555
to
72f03b1
Compare
No description provided.