Skip to content

fix: Orphaned sub-registry name resolution in get_registered_names()#198

Merged
feussy merged 1 commit intomainfrom
rf/registered-names-for-orphaned-registries
Apr 23, 2026
Merged

fix: Orphaned sub-registry name resolution in get_registered_names()#198
feussy merged 1 commit intomainfrom
rf/registered-names-for-orphaned-registries

Conversation

@feussy
Copy link
Copy Markdown
Collaborator

@feussy feussy commented Apr 23, 2026

Summary

Fixes get_registered_names() returning [] for models whose parent sub-registry has been orphaned by an add(..., overwrite=True) call.

When a sub-registry is replaced in the root (e.g. during a config reload), add() removes (root, name) from the old sub-registry's _registrations, breaking the chain. Models still referencing the old sub-registry dead-end at the empty _registrations and can no longer resolve their names — silently breaking any downstream logic that depends on registry-path lookup (e.g. cache evaluators keyed by model name).

What's included

_RegistryMixin._was_registered

  • New PrivateAttr(default=False) added alongside _registrations
  • Set to True in ModelRegistry.add() the first time a model is added to any parent
  • Distinguishes an orphaned sub-registry (was once registered, then replaced) from a fresh/local sub-registry (created but never added to any parent) — the latter must remain invisible to name resolution, as existing semantics require

get_registered_names() — orphan fallback branch

  • New elif isinstance(self, ModelRegistry) and self._was_registered and self.name: branch before return []
  • Orphaned sub-registries return [f"/{self.name}"], reconstructing their path from the immutable .name attribute
  • Works at arbitrary nesting depth automatically: only the topmost orphaned registry hits the new branch; intermediate registries still have intact _registrations pointing to it, so the existing recursion assembles the full path correctly
  • Three guards prevent false positives: isinstance (never fires for plain BaseModel overwrites), _was_registered (never fires for local unrooted registries), self.name (belt-and-suspenders against root, whose name is "")

What's NOT included

  • No changes to ModelRegistry.add() de-registration logic — the removal of old registrations on overwrite is intentional and tested
  • No changes to callers of get_registered_names() — all existing callers handle a non-empty return correctly

Tests

1 new test test_orphaned_subregistry_resolves_name covering the 2-level deep case (/foo/bar/model_name), with registry cleanup in finally. All 40 existing registry tests pass unchanged — including test_add_twice and test_add_two_places which verify that local unrooted sub-registries remain invisible.

test_pickle_consistency in test_base_serialize.py was updated to reflect that _was_registered is now included in __pydantic_private__ during pickling. The hard-coded expected bytes were regenerated accordingly; the test's purpose (proving deterministic pickle output across runs) is unchanged.

@feussy feussy force-pushed the rf/registered-names-for-orphaned-registries branch from cdadbae to 56cc780 Compare April 23, 2026 17:13
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

Test Results

650 tests  +2   648 ✅ +2   1m 45s ⏱️ -2s
  1 suites ±0     2 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 2274372. ± Comparison against base commit 4607ce4.

♻️ This comment has been updated with latest results.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #198      +/-   ##
==========================================
+ Coverage   95.98%   96.00%   +0.01%     
==========================================
  Files         140      140              
  Lines        9797     9827      +30     
  Branches      568      569       +1     
==========================================
+ Hits         9404     9434      +30     
  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 force-pushed the rf/registered-names-for-orphaned-registries branch from 56cc780 to f473ab1 Compare April 23, 2026 17:24
@ptomecek
Copy link
Copy Markdown
Collaborator

I think we need a flag to differentiate the different questions the method might be ansering

  • Is this model currently registered in the root registry, and if so, under what names?
  • Was this model ever registered in the root registry and if so, under what names?

I don't know what I would call such a parameter, current perhaps or include_previous? I would make the default equal to the past behavior.

…solve their registered names

Signed-off-by: Ryan Feuss <ryan.feuss@CubistSystematic.com>
@feussy feussy force-pushed the rf/registered-names-for-orphaned-registries branch from f473ab1 to 2274372 Compare April 23, 2026 18:39
@feussy
Copy link
Copy Markdown
Collaborator Author

feussy commented Apr 23, 2026

I think we need a flag to differentiate the different questions the method might be ansering

  • Is this model currently registered in the root registry, and if so, under what names?
  • Was this model ever registered in the root registry and if so, under what names?

I don't know what I would call such a parameter, current perhaps or include_previous? I would make the default equal to the past behavior.

Agreed, have added a flag and associated test that preserves existing behavior by default

@feussy feussy merged commit c291194 into main Apr 23, 2026
12 checks passed
@feussy feussy deleted the rf/registered-names-for-orphaned-registries branch April 23, 2026 18:49
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