Skip to content

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

Closed
feussy wants to merge 0 commit intomainfrom
rf/name_resolution_after_registry_serialization
Closed

fix: Ensure orphaned sub-registries can resolve names after pickling#200
feussy wants to merge 0 commit intomainfrom
rf/name_resolution_after_registry_serialization

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

@github-actions
Copy link
Copy Markdown
Contributor

Test Results

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

Results for commit 631cc87. ± 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 95.98%. Comparing base (e3a8b94) to head (631cc87).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #200      +/-   ##
==========================================
- Coverage   96.00%   95.98%   -0.02%     
==========================================
  Files         140      140              
  Lines        9827     9841      +14     
  Branches      569      569              
==========================================
+ Hits         9434     9446      +12     
- Misses        275      276       +1     
- Partials      118      119       +1     

☔ 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 closed this Apr 25, 2026
@feussy feussy force-pushed the rf/name_resolution_after_registry_serialization branch from 631cc87 to e3a8b94 Compare April 25, 2026 00:35
@feussy feussy deleted the rf/name_resolution_after_registry_serialization branch April 25, 2026 00:36
@feussy feussy restored the rf/name_resolution_after_registry_serialization branch April 25, 2026 00:37
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.

1 participant