Extend the use of config_expr #2409
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR extends the functionality of config_expr by allowing globals from the user’s call site to be captured and by adding a to_dict conversion for delayed evaluations.
- Propagate saved globals via the constructor for DelayEvaluator
- Introduce a new to_dict parameter in resolve_delayed_evaluator and update related usages
- Adjust delayed-evaluator resolution in decorators and parameters to incorporate the to_dict conversion
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| metaflow/user_configs/config_parameters.py | Updated DelayEvaluator to accept saved_globals; adjusted eval usage and config_expr to capture caller globals; added to_dict conversion in resolve_delayed_evaluator |
| metaflow/parameters.py | Updated resolve_delayed_evaluator calls to include to_dict conversion |
| metaflow/decorators.py | Updated delayed evaluator unpacking and re-resolution to use the to_dict flag |
Comments suppressed due to low confidence (1)
metaflow/user_configs/config_parameters.py:389
- [nitpick] Consider enhancing the docstring or adding inline comments for the new 'to_dict' parameter to clarify its behavior and when its conversion is applied.
def resolve_delayed_evaluator(v: Any, ignore_errors: bool = False, to_dict: bool = False) -> Any:
| @@ -157,8 +158,9 @@ class DelayEvaluator(collections.abc.Mapping): | |||
| of _unpacked_delayed_* | |||
| """ | |||
|
|
|||
| def __init__(self, ex: str): | |||
| def __init__(self, ex: str, saved_globals: Optional[Dict[str, Any]] = None): | |||
There was a problem hiding this comment.
[nitpick] The constructor now accepts a new parameter 'saved_globals'. Consider updating the class docstring to explain the purpose and usage of this parameter to help future maintainers.
Copilot uses AI. Check for mistakes.
|
Netflix internal testing[1112] @ d6298d5 |
Now allows: @Environment(vars=config_expr("...")) - This was broken since vars was a ConfigValue and not an updatable dictionary @project(name=config_expr("get_my_name(cfg)")) - This was broken because when evaluating the config_expr, we did not consider the globals at the point of call but instead inside the file defining config_expr. Tests are deployed internally.
- fixes a case where configs and Runner/Deployer were not compatible due to
configs not being processed in the same cwd
- fixes an issue with constructing the config values (introduced in
previous commit)
- fixes the error message when both default and specified config files
are not found
Previously something like:
RETRIES = config_expr("settings").retry
class MyFlow(FlowSpec):
@retries(times=RETRIES)
@step
def start(self):
...
@retries(times=RETRIES)
@step
def end(self):
...
would fail. Also, something like:
FOO = config_expr("settings").foo.bar
BAR = FOO.BAZ
BLAH = FOO.BAZ2
would also fail.
This fixes these issues by copying DelayEvaluator everytime and also
making the `__call__` function reentrant.
romain-intel
force-pushed
the
fix/configs_func
branch
from
d63a708 to
28b22ae
Compare
savingoyal
approved these changes
May 29, 2025
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now allows:
@Environment(vars=config_expr("..."))
@project(name=config_expr("get_my_name(cfg)"))
Tests are deployed internally.