Mousius commented on pull request #30:
URL: https://github.com/apache/tvm-rfcs/pull/30#issuecomment-927878667
Hi @areusch, responding to your comments now :smile_cat:
> thanks @Mousius for addressing my comments! i am basically good with this
proposal outside of the merging aspect. i think it would be good to consider
this a bit more--what i don't want to do is overcomplicate the `tvmc` tool and
make it difficult to use or for us to debug. in particular i think there are a
couple of aspects of config merging which aren't well-defined:
> * when should specifying a target parameter delete the previous target vs
append to the list of targets? it seems so far given the examples, the rules
are roughly
>
> * on the command lineN:
>
> * when `-target=` is given on the command line, the set of targets is
deleted
> * otherwise `-target-<name>-<attr>` is merged into the previous target.
> * in a config file:
>
> * if targets don't conflict: they are concatenated
> * if targets conflict: not sure I can tell, but i could be being dense
here.
The hierarchy proposed within the RFC is:
1. Arguments parsed by `argparse`
2. Configuration file specified (defaults to `default`)
3. Internal defaults for arguments in `tvmc`
This means the `tvmc` arguments will win out over config and both win out
over defaults. The motivation for this is similarly to not overcomplicate the
user interface and provide simple rules.
> here are some thoughts:
>
> * I don't think it's likely someone is going to want to split the config
at such as level as to require `--config=cortex-m4.json (`{"targets": {"kind":
"llvm", "mcpu": "cortex-m4"}}`) and --config=fpu.json` (`{"targets": {"kind":
"llvm", "mattr": "+fp"}}`) separately.
> * i think rather that someone may likely provide
`--config=boards/abc.json` along with `--config=executor/aot.json`.
> * i also think that by choosing JSON5, we are making it possible for
someone to easily write their own script or use e.g. `jq` to combine config
files. i'm not sure we need to handle all the cases here in the command-line
parser.
>
> i'd suggest the following tweak:
>
> * attrs can be overridden on the command line but not deleted (e.g. target
attrs or executor attrs)
> * otherwise, when merging configs, when combining top-level keys, you can
only completely override, not merge
> * if someone wants to do something fancier, they write their own script to
do it.
This RFC does not propose the merging of configuration files, instead
prioritising a single configuration - as you've mentioned this is more
complexity than we really need. The hierarchy from the RFC shown above should
cater to how overrides are allowed to work without providing much complexity?
So I think we're in agreement that users can then construct their own
configurations and only need one configuration - I'm just suggesting the CLI
serves are the final override point so you can load a config and change options
easily. What do you think @areusch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
