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
tickets/DM-17060 #4
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.
Looks OK, one suggestion for simplifying syntax of new command line argument.
subparser.add_argument("--dataset-name-substitution", metavar="LABEL:VALUE", | ||
dest='pipeline_actions', action='append', type=_ACTION_NAME_TEMPLATES, | ||
help='Configuration name template values, applies to a task with a given ' | ||
'label.') |
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.
Looking at the code I think VALUE in this case is a Python mapping of string to string. It may be rather messy to write that on command line and will need a lot of quoting, I suppose it should look like:
--dataset-name-substitution 'FancyTask:{"inputCoaddName": "deep2", "outputCoaddName": "deep2"}'
Could some other format with less quoting be more user-friendly, maybe something like:
--dataset-name-substitution FancyTask:inputCoaddName=deep2,outputCoaddName=deep2
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.
Jim and I debated this a little bit with pros and cons for several different ways. The conclusion we reached is that for now we will leave it as is, and collect data from several people in the group for the input pattern they would prefer, and make any changes in a future ticket.
@@ -91,6 +91,7 @@ def __call__(self, value): | |||
_ACTION_LABEL_TASK = _PipelineActionType("relabel", "(?P<label>.+):(?P<value>.+)") | |||
_ACTION_CONFIG = _PipelineActionType("config", "(?P<label>[^.]+)[.](?P<value>.+=.+)") | |||
_ACTION_CONFIG_FILE = _PipelineActionType("configfile", "(?P<label>.+):(?P<value>.+)") | |||
_ACTION_NAME_TEMPLATES = _PipelineActionType("name_templates", "(?P<label>[^.]+)[=](?P<value>.+)") |
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 use :
instead of =
for consistency with other options?
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 really have no opinion on this, I had thought having the equals sign make it more akin to setting a config option, as you are kind of assigning something to that task, but then again it is a different pattern because normally the label is just that a label not something that is assignable. So maybe it would be good to go back to : as the command line switch name already indicates that you are doing an action.
1c4024a
to
de4dff3
Compare
@@ -91,6 +91,7 @@ def __call__(self, value): | |||
_ACTION_LABEL_TASK = _PipelineActionType("relabel", "(?P<label>.+):(?P<value>.+)") | |||
_ACTION_CONFIG = _PipelineActionType("config", "(?P<label>[^.]+)[.](?P<value>.+=.+)") | |||
_ACTION_CONFIG_FILE = _PipelineActionType("configfile", "(?P<label>.+):(?P<value>.+)") | |||
_ACTION_NAME_TEMPLATES = _PipelineActionType("name_templates", "(?P<label>[^.]+):(?P<value>.+)") |
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.
That may not work too well if value
contains :
(as it should for dictionary). regexp is greedy by default and will match :
with rightmost :
in the string. Having unit test would probably catch it.
de4dff3
to
8e7d6df
Compare
Add Switch for setting Datatype name templates