-
Notifications
You must be signed in to change notification settings - Fork 3
Add mixin to allow pulling key-value pairs from python objects #128
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nijat Khanbabayev <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #128 +/- ##
==========================================
+ Coverage 95.42% 95.47% +0.05%
==========================================
Files 120 126 +6
Lines 6754 7295 +541
Branches 427 464 +37
==========================================
+ Hits 6445 6965 +520
- Misses 203 215 +12
- Partials 106 115 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
result = handler(values) | ||
|
||
# Attach debug metadata if spec was provided | ||
if isinstance(result, object) and spec is not None: |
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 there should be an option to disable this?
values[k] = v | ||
return values | ||
else: | ||
conflicts = [k for k, v in mapping.items() if k in values and values[k] != v] |
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.
arguably we could do this check in the in every case, and log the results.
This is a relatively big change, and I have to think about other ways of accomplishing this. For example, instead of the configs referencing python paths of other configs, if those other configs were in the registry already, then the existing mechanisms would work to reference them. Then the question becomes how to take some model instances that are defined in python and add them as named objects to the registry. I think this may be doable leveraging hydra's |
Add
PathKeyResolverMixin
which allows specifying some key/value pairs from python code via aPyObjectPath
. This allows offloading some of the configuration to python, getting benefits from:easier testability, cohesiveness of related configurations, and deduplication.
One specific workflow this helps with, is that partial functions can be constructed by taking a general CallableModel, and specializing (specifying specific/partial configuration options) it for various use-cases. The default
resolved_wins
, provides a (overrideable) check that pre-defined configurations in the code are not accidentally overwritten, and reduces the surface area exposed that could be overriden via yaml.This functionality is opt-in, since base
ccflow.BaseModel
classes do not inherit from it via default.Motivation (including in docs file added):