Open noklam opened 1 year ago
Just a few quick comments, without trying it out myself...
--params
as much as possible to OmegaConf.from_dotlist
. The :
syntax should be considered deprecated. See https://github.com/kedro-org/kedro/issues/1965kedro run --params a=[1, 2, 3]
. Since you can do OmegaConf.from_dotlist(["a=[1, 2, 3]"])
. If this doesn't already work then it's a (not very important) problem with our implementation of _split_params
@ankatiyar dotlist
syntax which we have not discussed before. i.e. how to do kedro run --params a=1,b=2
. Note that OmegaConf.from_dotlist
requires a list of strings (i.e. ["a=1", "b=2"]
rather than "a=1,b=2"
) so we'll still need to do some splitting or something hereJust to clarify: even though we should delegate to OmegaConf.from_dotlist
for this, it shouldn't be tried to the config loader in any way. We're just using their syntax for parsing params from the CLI.
@antonymilne what if indeed this (parsing the --params
) was offloaded to the config loader class 🤔? That would actually make this really customizable with sensible default.
FYI, in our plugins we're actually using JSON format to pass the params, e.g. https://github.com/getindata/kedro-azureml/blob/5ae7bcf430ec0a4824b6a198c4e919ae444e2589/kedro_azureml/cli.py#L236
That would actually make this really customizable with sensible default.
What do you mean? The proposal does not requires using ConfigLoader, it only use the OmegaConf cli parse feature. What extra feature is enabled to use config loader?
What extra feature is enabled to use config loader?
Someone could e.g. implement custom parsing of CLI args - like the JSON example.
A quick attempt as I come across some question in Slack today.
Originally I want to take regex approach https://stackoverflow.com/questions/26633452/how-to-split-by-commas-that-are-not-within-parentheses but without success.
https://github.com/kedro-org/kedro/pull/3327 - this is not ready to be merged. This only attempt to support []
but not {}
, I found it quite difficult to parse string correctly so I limit the scope for brackets only. On the other hand, list is more CLI friendly where typing out the entire dictionary in CLI is overcomplicated.
The key is try to parse the comma correctly, that is, ignoring comma inside []
()
{}
, the remaining part will basically just work because OmegaConf.from_dotlist
handles list and dict by default.
What's the status of this? 🤔
It was never put into a sprint, I made a draft PR to see what it takes to add just list as an example.
It requires building a parser basically, though in this case maybe we can just add list and forget about dict and everything.
Is there some prior art of CLI tools accepting Python primitive non-scalar types on the CLI?
Some overlap with this recent discussion https://github.com/kedro-org/kedro/pull/3904#issuecomment-2142457119
Description
Short description of the problem here. Some kedro cli like
kedro run --tags
accept argument with comma-separated list. This is not true with the--params
as,
get parsed as a separate parameters.kedro run --params error:1,2
orkedro run --params error:[1,2]
won't work`You will get some error like this.
The reason that I need this is I want to create my hooks, which takes CLI arguments and execute custom logic. The
--params
is the most intuitive argument I can find, and I can get it frombefore_pipeline_run
'srun_params
.The alternative is to add my custom CLI flag. This has 2 disadvantage.
run_params
is hard-coded This creates a tight couple between my cli file and my hooks. Usingparams
does not requires any of this, and the CLI already handle all the parsing (with OmegaConf) for me.Context
How has this bug affected you? What were you trying to accomplish? For some reason, I remember this works in older versions (??) or maybe the only way is to use the
--config
flag but that flag wasn't well used either. I want my hooks reading the CLI, one of the easiest way is to use the--params
. Even if I want to parse,
myself, the CLI will not allow it. My alternative is to use other delimeter like&
Steps to Reproduce
kedro run --params error:a=1,2
should return a dictionary like{"a": [1,2]}
Expected Result
Tell us what should happen. Maybe it never worked before, or the documentation is missing.
Actual Result
Tell us what happens instead. I get an error
Your Environment
Include as many relevant details about the environment in which you experienced the bug:
pip show kedro
orkedro -V
): 0.18.7python -V
): 3.9.1