fix(odin): Fix checkin worker picklable#556
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements custom __getstate__ and __setstate__ methods for CheckinWorker to enable pickling by dropping and recreating unpicklable threading.RLock instances. This resolves issues when spawning processes on macOS and Windows. Corresponding unit tests have been added to verify the pickling round-trip and behavior under the spawn start method. Feedback suggests terminating the spawned test process in the finally block if it remains alive to prevent leaked background processes in CI.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #556 +/- ##
==========================================
+ Coverage 83.76% 83.82% +0.05%
==========================================
Files 46 46
Lines 4627 4636 +9
==========================================
+ Hits 3876 3886 +10
+ Misses 751 750 -1
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds __getstate__ and __setstate__ methods to CheckinWorker to allow pickling when using spawn-based multiprocessing, which is the default on macOS and Windows. This is achieved by dropping the unpicklable RLock objects during pickling and recreating them upon unpickling. Additionally, unit tests are added to verify pickling round-trips and spawn-based process execution. The reviewer suggested using dict.pop(key, None) instead of del when removing locks from the state dictionary to prevent potential KeyErrors if the locks are not present.
vikramlc-cognite
added
the
waiting-for-risk-review
3 participants
Summary
Fixes CheckinWorker so it can be pickled, unblocking extractor startup on macOS/Windows where multiprocessing defaults to the spawn start method.
Type of change
What changed
(both threading.RLock) from the pickled state; setstate recreates fresh RLock instances after unpickling.
present on the branch, collapsing it to the single intended /files entry.
test_checkin_worker_is_picklable_with_spawn_start_method (see below).
Why it changed
is fork, which doesn't pickle args, so the bug is invisible in CI/Docker. On macOS and Windows, the default start method is spawn, which pickles
every argument. CheckinWorker held threading.RLock instances (_lock, _flush_lock), and RLock objects can't be pickled, so the extractor crashed
immediately on startup with TypeError: cannot pickle '_thread.RLock' object before any provider/destination code ran — a full local-dev blocker on
macOS/Windows.
What to focus on during review
updates/action updates, and active_revision) round-trips through pickling unchanged. Worth double-checking no other non-picklable attribute gets
added to CheckinWorker in the future without updating getstate.
anyway, so fresh locks are correct here rather than something to reconcile.
case you want them split into a separate commit, though they're trivial enough I folded them in.
Test evidence
survives, and that the recreated _lock/_flush_lock are new, distinct, usable RLock objects.
multiprocessing.get_context("spawn").Process(...) with a live CheckinWorker as a target arg, matching Runtime._spawn_extractor's exact usage.
applied.
KEYVAULT_TENANT_ID env var — confirmed failing on master too, unrelated to this change).
pre-existing constraint, not something this change affects; the two new tests were deliberately written to avoid needing those credentials.
Risks and unknowns
(Linux), which is why the bug was invisible there in the first place.
branch). Harmless to git (matches no files) but worth a follow-up cleanup PR.
Rollout and rollback
Checklist