Repository navigation
Correct pickling error check. - #40597
Conversation
There was a problem hiding this comment.
📋 Review Summary
This PR fixes a bug in _RunInferenceDoFn._run_inference where e is pickle.PickleError was used instead of isinstance, which always evaluated to False. It introduces a helper _is_pickling_error that checks for pickle.PickleError instances as well as TypeError and AttributeError exceptions containing 'pickle' in their message, accompanied by unit and pipeline tests.
🔍 General Feedback
- The implementation and unit tests are clean, well-structured, and accurately cover the ways Python's
picklemodule surfaces pickling failures across process boundaries.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40597 +/- ##
=============================================
+ Coverage 56.16% 79.91% +23.74%
=============================================
Files 1124 488 -636
Lines 178336 82388 -95948
Branches 1489 0 -1489
=============================================
- Hits 100170 65841 -34329
+ Misses 75645 16547 -59098
+ Partials 2521 0 -2521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
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). |
The
e is pickle.PickleErroralways evaluated to False since e is an instance of an exception.