fix(pipeline): replace private-attribute and name-based provider coupling with public contracts - #257
Merged
himanshu231204 merged 5 commits intoJul 29, 2026
Conversation
Add a public read-only LLMProvider.model_name property with a default implementation returning None so legacy providers keep working. Implement it on every concrete LLM provider in the repo to return the actual model identifier. Update Pipeline._run_metrics to use the public property while keeping the config fallback. Fixes OpenAgentHQ#51 Co-Authored-By: Kimi K2.7 Code <noreply@kimi.com>
…ntract Add ground_truth_contexts as an explicit keyword parameter to Retriever.retrieve with a None default. Update every concrete retriever in the repo to accept the parameter; retrievers that do not use it ignore it without changing behaviour. Remove the name == "mock" branch from Pipeline._retrieve and always forward the ground-truth contexts. Fixes OpenAgentHQ#53 Co-Authored-By: Kimi K2.7 Code <noreply@kimi.com>
Co-Authored-By: Kimi K2.7 Code <noreply@kimi.com>
…atibility Commit 73c2dc9 unconditionally passed ground_truth_contexts to every retriever. Third-party retrievers written against the legacy retrieve(query, k=5) signature therefore raised TypeError, which the pre-existing bare except Exception in _retrieve swallowed silently, causing retrieval to degrade to the dataset fallback on every call. Detect capability once per retriever using inspect.signature on the bound retrieve method, cache the result on the pipeline instance, and pass the keyword only when the retriever supports it (either explicitly or via **kwargs). The legacy two-argument form is called otherwise. The name == "mock" check remains removed. Add a regression test proving a legacy-signature retriever's documents reach the pipeline result. Co-Authored-By: Kimi K2.7 Code <noreply@kimi.com>
|
🎉 Congratulations @Nitjsefnie! Your pull request has been successfully merged into main. 🚀 Thank you for contributing to OpenAgentHQ and helping improve the project. We truly appreciate your contribution and hope to see you back with more amazing PRs! Happy Open Sourcing! ❤️ |
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.
Description
Two places where
core/pipeline.pyreached into provider internals instead of a declared contract. #51: the model name for metrics came from the privateself._llm._model, silently falling back to the config value for any provider naming that attribute differently. #53:_retrievebranched ongetattr(self._retriever, "name", None) == "mock"to decide whether to pass ground-truth contexts, so the core pipeline knew one retriever by name and no other retriever could ever opt in.Both are fixed by adding the missing public contract rather than removing anything: a
model_nameproperty on the LLM provider base, andground_truth_contextsas a real parameter onRetriever.retrieve().Type of Change
Related Issues
Closes #51
Closes #53
How Has This Been Tested?
Three regression tests, each verified to fail on the code it targets and pass after:
test_pipeline_uses_public_model_name_property— a fake provider exposingmodel_nameand not_model, with a model id deliberately different from the config's, so a silent fallback is distinguishable from a real read. Pre-fix:AssertionError: assert 'config-model' == 'provider-specific-model'.test_pipeline_passes_ground_truth_contexts_to_non_mock_retriever— asserts on the argument the retriever actually received. Pre-fix:AssertionError: assert None == ['ground truth context one', 'ground truth context two'].test_pipeline_keeps_legacy_retriever_working— a retriever with the old two-argument signature must still have its documents reach the result. Pre-fix (against the intermediate commit, before the compatibility fix):AssertionError: assert [] == ['legacy doc for What is RAG?'].Behaviour was also probed directly, per case: a retriever declaring the parameter receives it; one accepting
**kwargsreceives it; a legacy retriever is called without it and returns its real documents;MockRetrieverreceives it and behaves exactly as before.uv run pytest) — full run 1030 passed, 4 skipped; coverage 79.60% against the 75% floor.uv run ruff check .) — exits 1 on the same pre-existing findings asmain; no new finding on any file this PR touches.uv run mypy openagent_eval/) — not run; CI's step targets a non-existentsrc/, which is CI type-check step targets a non-existent src/ directory, so mypy has never run on the package #250.Checklist
Additional Notes
Why capability detection rather than just passing the new argument. Removing the
"mock"check means the pipeline passesground_truth_contextsto every retriever. All eleven retrievers in this repo were updated to accept it, so the suite goes green either way — but an out-of-treeRetrieversubclass written againstretrieve(self, query, k=5)would raiseTypeError, and_retrieve'sexcept Exceptionwould absorb it silently, degrading that user's retrieval to the dataset fallback on every call with nothing logged. Since this is a published package, the pipeline now inspects the retriever's signature once, caches the result, and passes the keyword only when it is actually accepted. A retriever taking**kwargscounts as accepting it. Verified the inspection runs exactly once for a pipeline's lifetime, including under the parallel executor.This is capability detection, not a name check — the pipeline still knows nothing about which retriever it holds, which is the point of #53.
Bug discovered while doing this work, filed separately:
That one is pre-existing (
pipeline.py:177onmain) and this PR does not change it — it only narrows what can raise inside thetry. It is the reason the compatibility hazard above was worth handling rather than documenting: with the swallow in place, the failure mode is invisible. Happy to send the logging fix as a follow-up; #256 has the detail.Known limitation, disclosed: if a retriever's
retrieveis decorator-wrapped such thatinspect.signaturereports**kwargswhile the underlying function does not accept the keyword, it will still raise into that same handler. And a callable whose signature cannot be introspected at all is treated as not supporting the parameter — safe, but currently silent. Both are noted in #256.If you'd rather not carry the introspection and prefer to declare the new parameter simply required of all retrievers, that is a reasonable call for a pre-1.0 package and I'll strip it — your repo, your compatibility policy.
Generated by Claude Opus 5 (brief, review), Kimi K2.7 Code (implementation), Claude Sonnet 5 (verification)