Preserve saved globals when copying DelayEvaluator - #3346
Conversation
Attribute and item access copy the evaluator and previously dropped caller globals, so config_expr with custom functions failed after chaining.
Greptile SummaryThis PR preserves the caller globals captured by
Confidence Score: 4/5The PR appears safe to merge, with only a non-blocking gap in the regression tests’ coverage of actual delayed evaluation. The implementation preserves the intended globals reference through every changed copy path, but the new tests would not detect a later failure in expression construction or evaluation because they only inspect private fields. Files Needing Attention: test/unit/test_delay_evaluator.py
|
| Filename | Overview |
|---|---|
| metaflow/user_configs/config_parameters.py | Correctly retains the saved globals reference across shallow and deep copies, matching the intended delayed-evaluation semantics. |
| test/unit/test_delay_evaluator.py | Covers all affected copy entry points, but asserts private state instead of reproducing successful delayed expression evaluation. |
Reviews (1): Last reviewed commit: "Preserve saved globals when copying Dela..." | Re-trigger Greptile
| def test_getattr_preserves_saved_globals(): | ||
| saved = _sample_globals() | ||
| evaluator = DelayEvaluator("config", saved_globals=saved) | ||
| chained = evaluator.project | ||
| assert chained._globals is saved | ||
| assert chained._access == ["project"] |
There was a problem hiding this comment.
Chained evaluation remains untested
These regression tests only inspect _globals and _access, so they do not exercise the __call__ evaluation path where the reported NameError occurred. Evaluating a chained expression and asserting its result would ensure future changes cannot preserve these private fields while breaking expression construction or namespace handling.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
PR Type
[x] Bug fix
[ ] New feature
[ ] Core Runtime change (higher bar; see CONTRIBUTING.md)
[ ] Docs / tooling
[ ] Refactoring
Summary
Attribute and item access on DelayEvaluator copy the object and previously dropped the caller globals captured by config_expr. Custom functions then raised NameError after any chained access.
Issue
Fixes #3345
Reproduction
Runtime: local
Commands to run:
Where evidence shows up: unit test output
Before (error / log snippet)
After (evidence that fix works)
Root Cause
copy and deepcopy constructed DelayEvaluator(self._config_expr) without saved_globals. getattr and getitem call copy, so chaining dropped the globals config_expr had stored.
Why This Fix Is Correct
Pass saved_globals=self._globals into both copy paths so the reference is kept, matching the previous comment that globals stay as a reference. Scope is limited to those two methods plus unit tests.
Failure Modes Considered
Tests
[x] Unit tests added/updated
[ ] Reproduction script provided (required for Core Runtime)
[ ] CI passes
[ ] If tests are impractical: explain why below and provide manual evidence above
Non Goals
No change to eval semantics beyond preserving the globals reference across copy.
AI Tool Usage
[ ] No AI tools were used in this contribution
[x] AI tools were used (describe below)
Cursor assisted with locating the bug from the issue report, applying the saved_globals pass through, and adding unit tests. All code was reviewed and the unit tests were run locally.