Repository navigation
[Python] Skip attributes that raise during stable code identifier lambda search - #41003
thelastbackspace wants to merge 1 commit into
Conversation
…bda search The lambda search in _search_module_or_class read every name from dir() with getattr, so an unrelated attribute that raises on access (an instance-only descriptor, a lazy PEP 562 module attribute) broke cloudpickle dumps with stable code identifier pickling enabled. Fixes apache#41002
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR fixes #41002 by catching Exception when reading attributes via getattr(node, name) during the lambda search in _search_module_or_class, preventing unrelated descriptors or lazy module attributes from crashing stable code identifier pickling. The implementation and unit test are clean, minimal, and well-targeted.
🔍 General Feedback
- Catching
Exception(rather thanBaseException) and continuing the loop in_search_module_or_classis the right approach and preserves fallback behavior when no matching lambda is found. - Minor test portability nit in
code_object_pickler_test.pyregarding module name prefix when executed directly as__main__.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
| def test_lambda_search_skips_attributes_that_raise(self): | ||
| fn = ClassWithLambdaAndRaisingAttribute.fn | ||
| path = code_object_pickler.get_code_object_identifier(fn) | ||
| self.assertEqual( |
There was a problem hiding this comment.
🟢 Low: Since ClassWithLambdaAndRaisingAttribute is defined in this test module, fn.__module__ will be '__main__' if the test file is executed directly via python code_object_pickler_test.py. Using f"{prefix}." (defined at line 141 as prefix = __name__) keeps this consistent with test_cases above.
| self.assertEqual( | |
| f"{prefix}." |
|
Assigning reviewers: R: @jrmccluskey for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
Fixes #41002
With stable code identifier pickling enabled,
_search_module_or_classwalksdir(node)and callsgetattron every name while looking for a lambda. If some unrelated attribute raises when read (a descriptor that only works on instances, or a PEP 562 lazy module attribute whose optional dependency is missing), that exception escapedget_code_object_identifierandcloudpickle_pickler.dumpsfailed, even though plain pickling of the same lambda works.An attribute that can't be read can't be the lambda we're looking for, so the search now just skips it and keeps going. If nothing matches, it still returns None as before and cloudpickle falls back to pickling by value.
Testing:
test_lambda_search_skips_attributes_that_raiseincode_object_pickler_test.py, using a class with an instance-only descriptor that sorts before the lambda indir(). It fails withTypeError: only valid on instanceswithout the change and passes with it.pytest apache_beam/internal/code_object_pickler_test.pyNote: #40711 also touches this file; happy to rebase if that lands first.