Skip to content

fix: Ensure orphaned sub-registries can resolve names after pickling#201

Merged
feussy merged 1 commit intomainfrom
rf/preserve_name_resolution_after_pickling
Apr 25, 2026
Merged

fix: Ensure orphaned sub-registries can resolve names after pickling#201
feussy merged 1 commit intomainfrom
rf/preserve_name_resolution_after_pickling

Conversation

@feussy
Copy link
Copy Markdown
Collaborator

@feussy feussy commented Apr 25, 2026

Summary

Fixes get_registered_names() returning [] for models whose _registrations chain passes through a deserialized RootModelRegistry.

get_registered_names() terminates the chain with [""] only when self is ModelRegistry.root() — an identity check against the process-level singleton _REGISTRY_ROOT. When a model is pickled (e.g. sent to a Ray worker via ray.remote) and unpickled, the RootModelRegistry stored in the _registrations chain is deserialized as a new Python object. The is identity check fails, the chain falls through to [], and all model names are silently lost. Any evaluator logic keyed by model name is bypassed without error.

What's included

RootModelRegistry.get_registered_names()

  • New override that unconditionally returns [""]
  • The root's semantic contract — I am the empty-prefix anchor of every path — is unconditional. It does not depend on which Python process constructed this object or whether this instance happens to be the live singleton
  • Placing the override directly on RootModelRegistry rather than patching the base-class identity check keeps the fix surgical: sub-registry resolution (_was_registered + orphan branch) is untouched

Why the existing _was_registered orphan branch does not cover this

  • Sub-registries correctly serialize _was_registered=True and their chains survive pickling intact — the orphan branch short-circuits at the first orphaned node and never reaches the root
  • RootModelRegistry is created via model_construct(), so _was_registered stays False and name is "" (falsy); even with _was_registered=True, the orphan branch guard self._was_registered and self.name would not fire for the root
  • No change to _was_registered or the orphan branch is needed or made

What's NOT included

  • No change to ModelRegistry.get_registered_names() — the base-class logic for live sub-registries and the include_orphaned path is correct and unmodified
  • No change to pickling/unpickling behavior — the fix tolerates deserialization rather than preventing it
  • No change to _REGISTRY_ROOT construction or the ModelRegistry.root() classmethod — the singleton pattern is preserved for all in-process uses

Tests

test_get_registered_names_survives_pickling (new, TestRegistry)

  • Registers a model at /foo/bar/baz in a nested 3-level sub-registry chain
  • Pickles and unpickles the model via pickle.dumps / pickle.loads, simulating Ray worker serialization of a standalone blob
  • Asserts get_registered_names() still returns ['/foo/bar/baz'] on the deserialized instance

…urn empty string so that resolving names for orphaned sub-registries works correctly after pickling

Signed-off-by: Ryan Feuss <ryan.feuss@CubistSystematic.com>
@github-actions
Copy link
Copy Markdown
Contributor

Test Results

651 tests  +1   649 ✅ +1   1m 46s ⏱️ +2s
  1 suites ±0     2 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 3f0d9cf. ± Comparison against base commit e3a8b94.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.00%. Comparing base (e3a8b94) to head (3f0d9cf).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #201   +/-   ##
=======================================
  Coverage   96.00%   96.00%           
=======================================
  Files         140      140           
  Lines        9827     9839   +12     
  Branches      569      568    -1     
=======================================
+ Hits         9434     9446   +12     
  Misses        275      275           
  Partials      118      118           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@feussy feussy merged commit 7115458 into main Apr 25, 2026
12 checks passed
@feussy feussy deleted the rf/preserve_name_resolution_after_pickling branch April 25, 2026 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants