config: Modify install_config_module to use a layered approach #138758
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/138758
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit ffcee60 with merge base b9618c9 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
This looks good at a high level, but some details (and possibly some of the ones I remarked on are causing the test failures) |
|
Need to fix inductor/test_config.py and related tests. update - done |
| config.update(changes) | ||
| prior = {k: config[k].user_override for k in changes} | ||
| for k, v in changes.items(): | ||
| self._config[k].user_override = v |
There was a problem hiding this comment.
This probably slightly regresses critical pass as the previous .update() call was implemented natively in CPython and now we have to do a loop in Python.
There was a problem hiding this comment.
Let me know if this is a blocker, I can make a "fast path" override dict if that's a concern.
There was a problem hiding this comment.
Unfortunately we don't seem to microbench this so I can't tell
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
This modifies the config system, to use a single mapping of config -> ConfigEntry and to store the default and user values within them. We could have used multiple dicts (i.e. user_override and default), but as we add more fields (justknobs in this PR, perhaps testing and env variables later), it quickly becomes painful. There are a couple design decisions we could change. 1) All configs we save store the resolved value - not the default and user override seperately 2) All configs we load, apply the resolved value as a user override. This means that certain complexities of default behvaiour and deletion (as well as JK), will change if you save + load a config. ghstack-source-id: bb4f1a6 Pull Request resolved: pytorch#138758
…ch#138758) This modifies the config system, to use a single mapping of config -> ConfigEntry and to store the default and user values within them. We could have used multiple dicts (i.e. user_override and default), but as we add more fields (justknobs in this PR, perhaps testing and env variables later), it quickly becomes painful. There are a couple design decisions we could change. 1) All configs we save store the resolved value - not the default and user override seperately 2) All configs we load, apply the resolved value as a user override. This means that certain complexities of default behvaiour and deletion (as well as JK), will change if you save + load a config. Pull Request resolved: pytorch#138758 Approved by: https://github.com/ezyang
3 participants
Stack from ghstack (oldest at bottom):
This modifies the config system, to use a single mapping of config ->
ConfigEntry and to store the default and user values within them.
We could have used multiple dicts (i.e. user_override and default), but
as we add more fields (justknobs in this PR, perhaps testing and env
variables later), it quickly becomes painful.
There are a couple design decisions we could change.
user override seperately
This means that certain complexities of default behvaiour and deletion
(as well as JK), will change if you save + load a config.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang @aakhundov