[LEADS-443] OPENAI_API_KEY environmental variable failure#260
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
Walkthrough
ChangesLazy Embedding Validation
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RagasMetrics
participant RagasEmbeddingManager
participant EmbeddingManager
Caller->>RagasMetrics: __init__(embedding_manager)
RagasMetrics->>RagasMetrics: store _embedding_manager, set _ragas_embedding_manager=None
Caller->>RagasMetrics: access embedding_manager
RagasMetrics->>RagasEmbeddingManager: construct from _embedding_manager
RagasEmbeddingManager->>EmbeddingManager: ensure_ready()
RagasMetrics->>RagasMetrics: cache RagasEmbeddingManager
Caller->>RagasMetrics: evaluate(scope)
RagasMetrics->>RagasMetrics: catch EvaluationError / EmbeddingError and return failure tuple
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lightspeed_evaluation/core/embedding/manager.py (1)
46-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the constructor return type and Google-style Args section.
__init__is part of the public API surface here, so it should include-> Noneand documentconfigin Google style.Proposed refactor
- def __init__(self, config: EmbeddingConfig): - """Initialize with config. Validation is deferred until ensure_ready() is called.""" + def __init__(self, config: EmbeddingConfig) -> None: + """Initialize with config. + + Args: + config: Embedding configuration. Validation is deferred until + ensure_ready() is called. + """As per coding guidelines,
src/**/*.pypublic functions and methods need type hints and Google-style docstrings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lightspeed_evaluation/core/embedding/manager.py` around lines 46 - 47, The public constructor in EmbeddingManager.__init__ is missing the explicit return annotation and Google-style parameter docs. Update __init__ to declare -> None and expand its docstring with a Google-style Args section documenting config, keeping the existing note about deferred validation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lightspeed_evaluation/core/embedding/manager.py`:
- Around line 64-68: The Embedding Manager startup log is exposing raw
provider_kwargs, which may contain secrets. Update the logging in the Embedding
Manager initialization/ready message to avoid printing the full provider_kwargs
dict; keep the provider and model, and either omit provider_kwargs entirely or
replace it with a safe summary (for example, only non-sensitive keys or a
count). Use the logger.info call in the Embedding Manager class to make this
change.
In `@src/lightspeed_evaluation/core/metrics/ragas.py`:
- Around line 97-100: The lazy initialization in evaluate via
RagasEmbeddingManager(self._embedding_manager) can now raise provider validation
errors that bypass the existing failure handling. Update evaluate in
RagasMetrics to catch the project’s embedding/configuration exceptions during
this manager creation path, using EmbeddingError and ConfigurationError (or the
shared exception base if available), so missing credentials return the existing
(None, "...failed...") result instead of aborting evaluation.
---
Nitpick comments:
In `@src/lightspeed_evaluation/core/embedding/manager.py`:
- Around line 46-47: The public constructor in EmbeddingManager.__init__ is
missing the explicit return annotation and Google-style parameter docs. Update
__init__ to declare -> None and expand its docstring with a Google-style Args
section documenting config, keeping the existing note about deferred validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2708eea4-352b-4fc3-9188-f453377c938b
📒 Files selected for processing (3)
src/lightspeed_evaluation/core/embedding/manager.pysrc/lightspeed_evaluation/core/embedding/ragas.pysrc/lightspeed_evaluation/core/metrics/ragas.py
|
@coderabbitai review |
xmican10
left a comment
xmican10
left a comment
There was a problem hiding this comment.
Thanks! I have two things:
RagasMetricsis shared across threads, so the lazyembedding_managerproperty andensure_ready()can race on first access. The codebase already useslitellm_state_lockfor the same pattern — should we add a lock here too?- The new lazy validation and exception handling don't have test coverage yet. I think we should add tests for
ensure_ready()idempotency/error cases, the lazy property behavior, and the new exception types in evaluate(), wdyt?
Thanks for reviewing it Eva,
|
Thanks for the clear up. Does this also apply to the Huggingface's embedding models which are downloaded and running on the local machine? |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/unit/core/metrics/test_ragas.py (1)
98-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd tests for
BrokenPipeErrorandOSErrorevaluate branches.
evaluate()has dedicated messaging for these branches, but the parametrized matrix here doesn’t cover them, so those paths can regress silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/core/metrics/test_ragas.py` around lines 98 - 130, The exception matrix in test_catches_exception_gracefully is missing coverage for the dedicated BrokenPipeError and OSError branches in RagasMetrics.evaluate. Extend the parametrized cases in test_catches_exception_gracefully to include BrokenPipeError and OSError with representative messages, and keep asserting that evaluate() returns None plus an error reason containing both the specific message and the generic “evaluation failed” text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lightspeed_evaluation/core/metrics/ragas.py`:
- Around line 77-79: Update the RagasMetrics.__init__ Google-style docstring to
reflect that embedding validation is deferred until lazy initialization rather
than completed up front. In the argument/contract text for embedding_manager,
remove language implying it is already validated and instead describe that it is
stored for later use by RagasEmbeddingManager and checked when the embedding
manager is first accessed. Use the RagasMetrics.__init__ and
_ragas_embedding_manager symbols to locate the docs and keep the wording aligned
with the new lazy behavior.
In `@tests/unit/core/embedding/test_manager.py`:
- Around line 19-31: The idempotency test for EmbeddingManager.ensure_ready() is
only patching _validate_config after the first two calls, so it can miss a
validation happening on the second call. Update test_second_call_is_noop in
test_manager.py to patch manager._validate_config before the first
ensure_ready() call, then invoke ensure_ready() twice and assert the validation
mock was called exactly once. Use the existing EmbeddingManager and
_validate_config symbols to keep the test focused on the idempotency behavior.
- Line 1: Remove the pylint suppression in test_manager and update the test to
validate the public behavior of the embedding manager instead of reading
protected state directly. In the test cases that currently access the protected
member, assert the observable retry outcome through the relevant manager methods
and retry-related side effects so the same contract is covered without
protected-access usage.
In `@tests/unit/core/metrics/test_ragas.py`:
- Line 1: The test module currently uses a file-level pylint suppression to mask
redefined-outer-name, too-many-arguments, and too-many-positional-arguments
warnings. Remove the global disable from the test file and refactor the affected
fixtures/test functions in test_ragas.py so their signatures no longer trigger
those lint rules, using the specific pytest fixture and test names in that file
to locate and update the problematic patterns.
---
Nitpick comments:
In `@tests/unit/core/metrics/test_ragas.py`:
- Around line 98-130: The exception matrix in test_catches_exception_gracefully
is missing coverage for the dedicated BrokenPipeError and OSError branches in
RagasMetrics.evaluate. Extend the parametrized cases in
test_catches_exception_gracefully to include BrokenPipeError and OSError with
representative messages, and keep asserting that evaluate() returns None plus an
error reason containing both the specific message and the generic “evaluation
failed” text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ac44954-98f5-4d45-9e3e-2d21601792a3
📒 Files selected for processing (5)
src/lightspeed_evaluation/core/embedding/manager.pysrc/lightspeed_evaluation/core/embedding/ragas.pysrc/lightspeed_evaluation/core/metrics/ragas.pytests/unit/core/embedding/test_manager.pytests/unit/core/metrics/test_ragas.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lightspeed_evaluation/core/embedding/ragas.py
- src/lightspeed_evaluation/core/embedding/manager.py
Yes...Same argument applies. No issues for any provider including HuggingFace local models. |
|
@bsatapat-jpg The impact is not about data corruption - rather memory usage, this is not a major concern for cloud provider. But for huggingface it is going to be a problem as we load the embedding model. If we use more threads, then the memory usage will spike. I see two options as follow up
cc: @xmican10 |
|
Added a lock to address the thread-safety concern for HuggingFace local models. The "gather metrics info at load time" approach is tracked as a follow-up. |
|
@bsatapat-jpg wrote:
Yes, IMO the best solution for now. You really don't want to have any race(s) in the code. @asamal4 wrote:
Not true for this particular scenario at least on Linux. Linux uses copy on write for memory allocation, the embedding models are typically read-only. However I definitely agree that the problem is real and it has to be solved. |
Thanks Kada... Added a threading.Lock with double-checked locking on the embedding_manager property. |
4 participants
Description
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
ensure_ready()and improved initialization readiness behavior.