fix: look up custom model postprocessing by resolved name - #700
fix: look up custom model postprocessing by resolved name#700Ramnath0521 wants to merge 1 commit into
Conversation
`CustomTextEmbedding.__init__` read `POSTPROCESSING_MAPPING` with the
`model_name` argument, but the mapping is keyed by the name the model was
registered under, and model lookup is case-insensitive. Registering
`Org/Model` and instantiating `org/model` therefore raised:
KeyError: 'org/model'
`self.model_description` is already resolved by the base class through the
case-insensitive matcher, so its `.model` is the canonical registered name.
Use that.
The autouse test fixture also now clears `POSTPROCESSING_MAPPING` alongside
`SUPPORTED_MODELS`; it reset only the latter, so a registration leaked into
subsequent tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Custom embedding models now retain their registered pooling and normalization settings when instantiated with different casing. The supported cases are covered, with no remaining merge-blocking risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Fixes #650.
What was wrong
CustomTextEmbedding.__init__looked up its postprocessing config with themodel_nameargument:But
POSTPROCESSING_MAPPINGis keyed bymodel_description.model— the name the model was registered under (add_model) — while model lookup is case-insensitive (_get_model_descriptioncomparesmodel_name.lower() == model.model.lower()). So the caller's spelling need not match the registered key.Reproduction
On
main:Resolution itself is fine —
TextEmbedding._get_model_description("org/model").modelcorrectly returns"Org/Model". Only the mapping lookup uses the raw argument.The change
self.model_descriptionis already set by the base class from that same case-insensitive matcher, so.modelis the canonical registered name. The lookup now uses it, and reads the entry once instead of twice.Tests
Two cases in
tests/test_custom_models.py:test_custom_model_postprocessing_lookup_is_case_insensitiveOrg/Model, instantiateorg/model— fails onmainwith theKeyErrorabovetest_custom_model_postprocessing_lookup_with_matching_case_still_worksBoth use
lazy_load=True, so they exercise the constructor without downloading weights.One extra change, called out
The autouse
restore_custom_models_fixtureresetSUPPORTED_MODELSbut notPOSTPROCESSING_MAPPING, so a registration leaked into every later test in the file. Adding a test that registers a model made that visible, so the fixture now clears both. Happy to split it out if you would rather it landed separately.Checks run locally
Windows 11, Python 3.12:
pytest tests/test_custom_models.py— 7 passed (5 existing + 2 new)mypy fastembed --disallow-incomplete-defs --disallow-untyped-defs --disable-error-code=import-untyped— 5 errors, all pre-existing invocab_resolver.py,preprocessor_utils.pyandcolbert.py; none in the changed file, and identical on an unmodifiedmainruff checkon both changed files — 5 findings, all pre-existing (UP035/I001import style), byte-identical on an unmodifiedmain;ruff format --check— already formattedI did not run the full
pytest tests/here: it downloads ONNX weights for every supported model and takes ~20 hours on this machine. This change touches only the custom-model constructor path, which the tests above cover directly.All Submissions
pre-commit— not installed locally; ranruff checkandruff format --checkdirectly insteadAuthored by Claude (an AI coding agent) on the account owner's machine and with their authorization; they have reviewed the change and confirmed it. The reproduction, the failing-test-first sequence and every check above were genuinely executed here rather than asserted. Flagging the AI authorship plainly rather than leaving it to be inferred — happy to take any correction in review.