-
Notifications
You must be signed in to change notification settings - Fork 22
add common custom llm #70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a new BaseCustomLLM class and exposes it publicly; generalizes parameter names from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Evaluator
participant Metrics as Metrics/Manager
participant Mgr as LLMManager
participant Custom as BaseCustomLLM
participant Lite as litellm
Evaluator->>Metrics: evaluate(...)
Metrics->>Mgr: get_model_name(), get_llm_params()
Mgr-->>Metrics: model_name, llm_params
Metrics->>Custom: instantiate(model_name, llm_params)
Metrics->>Custom: call(prompt, n=1, temperature=?, return_single=True)
Custom->>Lite: completion(model, messages, temperature, n, max_tokens, timeout, num_retries, ...)
Lite-->>Custom: response(choices[*].message.content)
Custom-->>Metrics: "text" or ["texts"]
note right of Custom: on error -> raise LLMError
sequenceDiagram
autonumber
actor RagasFlow
participant RagasMgr
participant RagasCustom
participant BaseCustom
participant Lite
RagasFlow->>RagasMgr: generate_text(prompt, n, temperature)
RagasMgr->>RagasCustom: generate_text(...)
RagasCustom->>BaseCustom: call(prompt, n, temperature, return_single=False)
BaseCustom->>Lite: completion(...)
Lite-->>BaseCustom: response
BaseCustom-->>RagasCustom: list[string] texts
RagasCustom-->>RagasMgr: LLMResult (Generation objects)
note right of RagasCustom: is_finished(response) -> True
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (4)src/lightspeed_evaluation/core/metrics/custom.py (3)
src/lightspeed_evaluation/core/llm/ragas.py (2)
src/lightspeed_evaluation/core/llm/deepeval.py (1)
tests/unit/core/llm/test_manager.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lightspeed_evaluation/core/llm/custom.py (1)
56-75: Consider more specific exception handling.The broad
except Exceptionat Line 74 catches all exceptions, includingKeyboardInterruptandSystemExit. While this is wrapped inLLMError, it could mask critical system-level issues during debugging. Consider catching more specific exceptions that litellm might raise (e.g.,litellm.exceptions.APIError,litellm.exceptions.Timeout).If litellm provides specific exception types, consider refactoring:
try: response = litellm.completion(**call_params) # ... response processing ... - except Exception as e: - raise LLMError(f"LLM call failed: {str(e)}") from e + except (litellm.APIError, litellm.Timeout, ValueError) as e: + raise LLMError(f"LLM call failed: {str(e)}") from e + except Exception as e: + # Log unexpected exceptions for debugging + raise LLMError(f"Unexpected LLM error: {str(e)}") from esrc/lightspeed_evaluation/core/metrics/custom.py (1)
67-72: Simplify unnecessary list handling.At Line 70-71, there's a check for
isinstance(result, list)even thoughreturn_single=Trueis passed toself.llm.call(). According to theBaseCustomLLM.call()implementation, whenreturn_single=Trueandn=1, it always returns a single string, never a list.Simplify the method:
def _call_llm(self, prompt: str) -> str: """Make an LLM call with the configured parameters.""" - result = self.llm.call(prompt, return_single=True) - if isinstance(result, list): - return result[0] if result else "" - return result + return self.llm.call(prompt, return_single=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/lightspeed_evaluation/core/llm/__init__.py(2 hunks)src/lightspeed_evaluation/core/llm/custom.py(1 hunks)src/lightspeed_evaluation/core/llm/deepeval.py(2 hunks)src/lightspeed_evaluation/core/llm/manager.py(3 hunks)src/lightspeed_evaluation/core/llm/ragas.py(4 hunks)src/lightspeed_evaluation/core/metrics/custom.py(4 hunks)src/lightspeed_evaluation/core/metrics/deepeval.py(1 hunks)src/lightspeed_evaluation/core/metrics/ragas.py(1 hunks)tests/unit/core/llm/test_manager.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/lightspeed_evaluation/core/metrics/ragas.py (1)
src/lightspeed_evaluation/core/llm/manager.py (2)
get_model_name(91-93)get_llm_params(95-103)
src/lightspeed_evaluation/core/llm/__init__.py (1)
src/lightspeed_evaluation/core/llm/custom.py (1)
BaseCustomLLM(10-75)
src/lightspeed_evaluation/core/llm/deepeval.py (1)
src/lightspeed_evaluation/core/llm/ragas.py (2)
get_llm(95-97)get_model_info(99-104)
tests/unit/core/llm/test_manager.py (1)
src/lightspeed_evaluation/core/llm/manager.py (1)
get_llm_params(95-103)
src/lightspeed_evaluation/core/llm/custom.py (1)
src/lightspeed_evaluation/core/system/exceptions.py (1)
LLMError(24-25)
src/lightspeed_evaluation/core/metrics/deepeval.py (1)
src/lightspeed_evaluation/core/llm/manager.py (2)
get_model_name(91-93)get_llm_params(95-103)
src/lightspeed_evaluation/core/llm/ragas.py (2)
src/lightspeed_evaluation/core/llm/custom.py (2)
BaseCustomLLM(10-75)call(18-75)src/lightspeed_evaluation/core/system/exceptions.py (1)
LLMError(24-25)
src/lightspeed_evaluation/core/metrics/custom.py (3)
src/lightspeed_evaluation/core/llm/custom.py (2)
BaseCustomLLM(10-75)call(18-75)src/lightspeed_evaluation/core/llm/manager.py (2)
get_model_name(91-93)get_llm_params(95-103)src/lightspeed_evaluation/core/system/exceptions.py (1)
LLMError(24-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: mypy
🔇 Additional comments (8)
src/lightspeed_evaluation/core/metrics/deepeval.py (1)
29-29: LGTM!The parameter retrieval correctly uses the renamed
get_llm_params()API, aligning with the generalized LLM parameter handling introduced in this PR.src/lightspeed_evaluation/core/metrics/ragas.py (1)
39-39: LGTM!The parameter retrieval correctly uses the renamed
get_llm_params()API, consistent with the broader refactor.src/lightspeed_evaluation/core/llm/manager.py (1)
95-103: LGTM!The method rename from
get_litellm_paramstoget_llm_paramscorrectly generalizes the API, making it provider-agnostic while maintaining the same return structure and parameter semantics.tests/unit/core/llm/test_manager.py (1)
69-91: LGTM!The test correctly validates the renamed
get_llm_params()method and ensures all expected parameter keys and values are returned.src/lightspeed_evaluation/core/llm/deepeval.py (1)
14-26: LGTM!The parameter handling correctly transitions from
litellm_paramstollm_params, maintaining consistency with the broader refactor while preserving the same initialization logic.src/lightspeed_evaluation/core/metrics/custom.py (2)
39-41: LGTM!The initialization correctly uses
BaseCustomLLMwith model name and LLM parameters from the manager, properly abstracting the LLM layer.
218-230: LGTM!The error handling correctly catches
LLMErrorand returns an informative message, improving robustness when LLM calls fail.src/lightspeed_evaluation/core/llm/custom.py (1)
44-53: Avoid passing None for optional parameters
Ensuremax_tokensandtimeoutaren’t set toNoneincall_params—either omit keys when their values areNoneor supply explicit defaults—so you don’t inadvertently passNoneintolitellm.completion.
a028301 to
1aab90d
Compare
1aab90d to
0871015
Compare
|
@VladimirKadlec @tisnik PTAL |
VladimirKadlec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently have the following classes holding the same params -- the params for llm -- and propagating them to the lower level:
(Config.LLMConfig)->RagasLLMManager->RagasCustomLLM->finally calls litellm.compltetition
This PR adds another guy to the chain (we have to go deeper :-) ) :
(Config.LLMConfig)->RagasLLMManager->RagasCustomLLM->BaseCustomLLM->finally calls litellm.completition
Not sure why we need it, just consider if the path from the config to the LLM instance is not too long.
Other than that, LGTM.
|
@VladimirKadlec Regarding |
tisnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Create a common module for custom llm to have an abstract layer for litellm.
This can be anything in future (so no need to mention litellm everywhere)
Summary by CodeRabbit
New Features
Refactor
Improvements
Tests