-
Notifications
You must be signed in to change notification settings - Fork 81
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
Command Line Configuration Files #30
Conversation
Can you address the common usage of environment variables for configuration, why that wasn't considered, and the potential for future work? |
Hi @hogepodge, that's actually a great thing to include in the future possibilities (and I've done so). The reason it's not included is purely to scope the work down to what seemed a reasonable chunk, I don't disagree that environment variables would be a great addition but would likely come with their own rules and nuances to add to the functionality. I've linked to Terraform, as an example of where all three work really well in harmony in the future possibilities. I don't think I need to include them in the alternatives as they're additive in my mind, does that sound right to you? |
I think the usage of environment variables for configuration of this kind in an AoT compilation flow with tvmc is quite debatable with the kinds of things it takes. . Personally I think its cleaner to consider this directly in configuration files of this kind. I have had a read through and this looks pretty good LGTM @leandron could you please take a look ? |
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.
LGTM. I think this will be a great improvement for new TVM/tvmc
users, without taking out the flexibility from the advanced users.
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.
@Mousius thanks for this RFC. I would welcome --config
option to tvmc
. I'm wondering if there are possibilities of providing PCB-level configs this way--hence my question about merging configs.
Hi @areusch, thanks for your feedback!
This is one of our core use-cases, which is why we have the folder I've addressed your other comments, could you take another look? |
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.
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.
- when
- 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.
- on the command lineN:
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.
[guide-level-explanation]: #guide-level-explanation | ||
|
||
## TVM Hosted Configurations | ||
Configurations will be stored as [JSON5](https://json5.org/) at `configs/<TYPE>/<NAME>.json`, this top level directory will enable other tooling to load configurations just as easily a `tvmc` and provide easy sign posting for users looking for configurations. |
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.
what can <TYPE>
be?
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.
It's intentionally not defined to allow for users to populate it as they see fit, I don't want to constrain the folders in that directory to requiring an RFC update. I provided configs/boards
as an example use case but I imagine there'll be many ways of subdividing 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.
ok can we add this note to the PR?
```json | ||
{ "autotuning_runs": 10 } | ||
``` | ||
Which would then be extended with `--config=default` (`{ "targets": [{ "kind": "llvm" }], "executor": { "kind": "graph", "system-lib": true } }`): |
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.
how does the "executor" schema map to the target string? targets seems straightfoward, but i think executor needs a refactor we haven't done yet (splitting the executor config out from Target).
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 is demonstrating a hypothetical configuration layout rather than a concrete layout, in this case I'm using the improvements from Migrating Target Attributes to IRModule to allow for a more detailed example. In this case it doesn't make a difference as the example is for the merging of portions of the JSON rather than any specific 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.
got it. can we add that as a note here?
Hi @areusch, responding to your comments now 😸
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? |
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.
ok great i see @Mousius! i think there are just a few last behaviors i want to make sure we align on:
- if the config file specifies "targets" key, all targets from the default config are deleted/overridden.
- if the command-line supplies
--targets=
, all targets from the config file are deleted/overridden. - if the command-line only supplies e.g.
--target-llvm-mcpu
, then it modifies the llvm target from config/defaults. - if the command-line or a config file specifies a target sub-key, it always overrides it in full (e.g. there is no appending to
mattr
from the command line).
[guide-level-explanation]: #guide-level-explanation | ||
|
||
## TVM Hosted Configurations | ||
Configurations will be stored as [JSON5](https://json5.org/) at `configs/<TYPE>/<NAME>.json`, this top level directory will enable other tooling to load configurations just as easily a `tvmc` and provide easy sign posting for users looking for configurations. |
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.
ok can we add this note to the PR?
```json | ||
{ "autotuning_runs": 10 } | ||
``` | ||
Which would then be extended with `--config=default` (`{ "targets": [{ "kind": "llvm" }], "executor": { "kind": "graph", "system-lib": true } }`): |
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.
got it. can we add that as a note here?
@areusch yip yip yip, I've added this to the document and referenced supporting RFCs 😸 |
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.
LGTM
No description provided.