-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
parsing: Support dict unpacking in cmd
.
#7907
Conversation
Any recent motivations for this? 🙂 |
When exploring how to better integrate |
This comment was marked as resolved.
This comment was marked as resolved.
cc627fb
to
b9daa31
Compare
6a76372
to
a2ea47d
Compare
a2ea47d
to
e4af3f4
Compare
def _(obj: dict): | ||
result = "" | ||
for k, v in flatten(obj).items(): | ||
if isinstance(v, bool): |
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 only works for argparse.BooleanOptionalAction
not for store_true/false
. Maybe need some better way to handle this?
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.
The same issue about different options, as commented below #7907 (comment)
Which option do you think we should consider as the most appropriate default?
Note that this can be used for things beyond argparse
, for example you can use the interpolation to pass flags to arbitrary executables that you call inside cmd
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.
Added config.parsing.list
with store_true
and boolean_optional
.
Not sure about store_false
because the interaction with dvc params would look kind of strange, IMO
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 also have no idea about 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.
I think that's more than enough to support for now.
dvc/parsing/interpolate.py
Outdated
raise ParseError( | ||
f"Cannot interpolate nested iterable in '{k}'" | ||
) | ||
result += f"--{k} {i} " |
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.
> import argparse
> parser = argparse.ArgumentParser()
> parser.add_argument('--foo', nargs='*')
> parser.parse_args('--foo x y'.split())
Namespace(foo=['x', 'y'])
> parser.parse_args('--foo x --foo y'.split())
Namespace(foo=['y'])
Looks like setting arguments for multi times will overwrite the previous one.
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.
The idea was to cover the action='append'
mode:
>>> parser = argparse.ArgumentParser()
>>> parser.add_argument('--foo', action='append')
>>> parser.parse_args('--foo 1 --foo 2'.split())
Namespace(foo=['1', '2'])
Maybe it's a matter of clearly stating it in the docs.
Ideally, we should support all options but I am not sure how to do it. We could add a bunch of config
options or just allow users to register a custom to_str
method for interpolation.
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.
Let's pick one behavior for now. Is it possible to see which one is more widely used in existing ML CLIs? I would guess the nargs
--foo 1 2
behavior is more common, even though I find the explicit append
behavior easier to read.
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.
Added config.parsing.list
with nargs
and append
options. It doesn't look that bad to have the 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.
Just noticed this, it's a bad idea to make dvc.yaml
parsing conditional, it should be self-contained.
e4af3f4
to
b41e148
Compare
|
||
|
||
@pytest.mark.parametrize( | ||
"bool_config", [None, "store_true", "boolean_optional"] |
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.
- Maybe we only need to test two different conditions? The
None
can be tested through some other method (Test if the default value is set correctly)? - Because these two configurations are independent, maybe we can test them in an (I'm not sure how to describe it) instead of using a product. ["a1b1", "a2b2"] instead of ["a1", "a2"] x ["b1", "b2"].
The two suggestions above can reduce the amount of the test from 9 to 2.
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 updated it to 3 test cases (["a1b1", "a2b2"]
). I still included the default in the same test, not sure if it's worth it to separate it as the body of the test would be the same
LGTM for the most part, another concern is that the two new configurations still can't solve all of the problems as two different types of |
Allow to use dictionaries as values for template interpolation but only inside the `cmd` key. See tests/func/parsing/test_interpolated_entry.py::test_cmd_dict for detailed syntax. Add `config.parsing` section for configuring behavior of ambiguous data types like booleans and lists.
b41e148
to
7fac181
Compare
Indeed. There are many unsolved issues but the idea for this initial P.R. is to cover basic scenarios to don't overcomplicate the logic until users request for specific issues |
config: Add `parsing` section. Per iterative/dvc#7907
config: Add `parsing` section. Per iterative/dvc#7907
Allow to use dictionaries as values for template interpolation but only inside the
cmd
key.Given the following params.yaml and dvc.yaml:
The dictionary will be unpacked with the following syntax:
Closes #6107
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.