Repository navigation
fix: Fall back to the serialized body when UDF source misses globals - #6816
Open
Daksha1611 wants to merge 5 commits into
Open
Daksha1611 wants to merge 5 commits into
Daksha1611 wants to merge 5 commits into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
Source-first rehydration execs only the UDF's own source into a namespace seeded with pandas/numpy. A UDF that reads a helper, constant, or aliased import from its defining module has no way to resolve those names, but the exec still succeeds, because a function body's free variables are only looked up when it is called. resolve_udf therefore treated rehydration as successful and never fell back to the dill body, even when that body carried the captured globals and would have run. The failure surfaced later as a NameError during retrieval, so an on-demand feature view that served correctly before started raising once it was loaded from the registry. Validate the rebuilt callable before accepting it: walk its LOAD_GLOBAL operands, and the operands of nested code objects, and return None when a name resolves in neither the exec namespace nor builtins. resolve_udf then falls back as it did before. Only LOAD_GLOBAL is inspected. co_names also holds attribute names, so checking it would flag working UDFs and push them onto the dill path for no reason. Self-contained UDFs are unaffected and still take the source path, keeping the Spark and cross-Python-version benefits intact. Signed-off-by: Daksha1611 <mehtadaksha1611@gmail.com>
Copy link
Copy Markdown
|
⚠️ Please install the Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #6816 +/- ##
==========================================
- Coverage 47.08% 47.08% -0.01%
==========================================
Files 419 419
Lines 51878 51875 -3
Branches 7525 7525
==========================================
- Hits 24429 24427 -2
+ Misses 25700 25699 -1
Partials 1749 1749
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What this PR does / why we need it:
Source-first UDF rehydration execs only the UDF's own source into a namespace seeded
with pandas/numpy. A UDF that reads a helper, constant, or aliased import from its
defining module cannot resolve those names — but the exec still succeeds, because a
function body's free variables are only looked up when the function is called.
resolve_udf therefore treated rehydration as successful and never fell back to the
dill body, even when that body carried the captured globals and would have run. The
failure surfaced later as a NameError during retrieval, so an on-demand feature view
that served correctly before started raising once it was loaded from the registry:
This validates the rebuilt callable before accepting it: walk its LOAD_GLOBAL
operands, plus those of nested code objects (inner functions, comprehensions), and
return None when a name resolves in neither the exec namespace nor builtins.
resolve_udf then falls back to the serialized body exactly as it did before.
Only LOAD_GLOBAL operands are inspected. co_names also contains attribute names
(df.columns), so checking that instead would flag working UDFs and push them onto the
dill path for no reason — there's a regression test covering that.
Self-contained UDFs are unaffected and still take the source path, so the Spark
segfault and cross-Python-version benefits of source-first rehydration are kept intact.
Which issue(s) this PR fixes:
Fixes #6815
Checks
Testing Strategy
Four regression tests added to sdk/python/tests/unit/transformation/test_udf_rehydrate.py.
Three of them fail on master and pass here; the fourth (test_attribute_access_is_not_ mistaken_for_a_missing_global) passes both ways by design, guarding the co_names
false-positive trap described above.
Verified end to end as well: an ODFV applied from a script, then read back in a separate
process holding only the registry, returned NameError before this change and the
correct value after.
sdk/python/tests/unit filtered to the transformation / on-demand / feature-view areas:
314 passed, 5 skipped. The 9 errors in that run are MongoDB testcontainers failing to
start locally and are present on master too.
ruff check, ruff format --check and mypy are clean on both changed files.
Misc
The pre-existing test_resolve_udf_prefers_source_over_garbage_dill_body still passes,
which confirms self-contained UDFs continue to prefer source over the body.