Centralize renderer registration metadata - #146
Open
hallerite wants to merge 1 commit into
Open
Conversation
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR centralizes routing, modality, lazy-import, and config-registration behavior behind a new manifest and dynamic loader, affecting shared runtime initialization across the package. Structural tests provide coverage, but the breadth of the abstraction and changed loading mechanics merit human review. Notes:
You can add or adjust custom eligibility rules. Learn more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RendererSpecmanifest for renderer names, loaders, config classes, model IDs, aliases, and modalitiesRendererConfigremains explicit for static typing, with an exact manifest consistency guard. Generated model routing and modality maps are unchanged frommain.Validation
9763 passed, 57 skipped(tests/test_multimodal.pyexcluded because processor loading requires unavailable Hugging Face network access in the local sandbox)ruff check .origin/mainNote
Low Risk
Refactor-only with structural tests and claimed byte-identical routing/modality maps; risk is accidental drift when editing the manifest, not runtime behavior change.
Overview
Introduces a single declarative manifest in
renderers/registry.py(RendererSpec/ModelSpec/RENDERER_SPECS) that records renderer names, import paths, config classes, HF model IDs, aliases, and multimodal modalities.renderers.base,renderers.configs, andrenderers.__init__no longer maintain parallel hand-edited maps.MODEL_RENDERER_MAP,MULTIMODAL_MODELS, lazy renderer imports,_CONFIG_BY_NAME, and_populate_registryare all built from the manifest. Aliases route like their representative checkpoint and inherit modalities;MODEL_PARITY_REPRESENTATIVESties every mapped ID to a parity-tested model.RendererConfigpydantic variants stay explicit for typing;tests/test_registry.pylocks manifest ↔ runtime metadata ↔ config union consistency and checks representatives exist inMODEL_CATALOG.Reviewed by Cursor Bugbot for commit 88973d5. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Centralize renderer metadata into
RENDERER_SPECSregistryRendererSpecandModelSpecdataclasses plus theRENDERER_SPECStuple that describes all renderers, their modules, classes, configs, and covered models with aliases and modalities_LAZY_RENDERERS,MODEL_RENDERER_MAP,MULTIMODAL_MODELS,_populate_registry, and_CONFIG_BY_NAMEwith dict comprehensions that derive their entries fromRENDERER_SPECSat import time_validate_specs()runs at import time and raisesValueErroron duplicate renderer names, classes, or model IDs, or unknown modality valuesRENDERER_SPECSand that_populate_registryloads every declared rendererrenderers.registrynow raisesValueErrorat import time if any spec has a duplicate name, class, or model ID — a previously-silent conditionMacroscope summarized 88973d5.