feat(langfuse): Add tracing opt-in/opt-out support#996
Open
vprashrex wants to merge 4 commits into
Open
Conversation
- Updated functions to accept `Langfuse | None` to handle cases where tracing is disabled. - Modified dataset upload and evaluation processing to gracefully handle scenarios without Langfuse. - Introduced `get_tracing_client` to manage Langfuse client initialization based on project settings. - Enhanced error handling and logging for tracing-related operations. - Updated tests to cover new behavior for tracing opt-out scenarios and ensure proper functionality.
📝 WalkthroughWalkthroughThis PR adds project-level tracing settings storage and a PATCH update path, introduces tracing-aware credential/client helpers, and threads optional tracing through request handling and evaluation flows with object-store dataset fallbacks and cached-data read paths. ChangesTracing settings and project API
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
changed the title
OpenAPI changes 🟢 6 non-breaking changesTip Safe to merge from an API-contract perspective. Full changelog ·
|
| Method | Path | Change | |
|---|---|---|---|
| 🟢 | GET |
/api/v1/projects |
added the optional property data/anyOf[subschema #1]/items/settings to the response with the 200 status |
| 🟢 | POST |
/api/v1/projects |
added the optional property data/anyOf[subschema #1: ProjectPublic]/settings to the response with the 200 status |
| 🟢 | GET |
/api/v1/projects/organization/{org_id} |
added the optional property data/anyOf[subschema #1]/items/settings to the response with the 200 status |
| 🟢 | PATCH |
/api/v1/projects/settings |
endpoint added |
| 🟢 | GET |
/api/v1/projects/{project_id} |
added the optional property data/anyOf[subschema #1: ProjectPublic]/settings to the response with the 200 status |
| 🟢 | PATCH |
/api/v1/projects/{project_id} |
added the optional property data/anyOf[subschema #1: ProjectPublic]/settings to the response with the 200 status |
main ↔ 51815fc0 · generated by oasdiff
vprashrex
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/services/evaluations/evaluation.py (1)
212-212: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring: Step 1 no longer requires "Langfuse ID".
The validation at Lines 266-270 now passes when the dataset has either
langfuse_dataset_idorobject_store_url, but the docstring still states the dataset must "exist and have Langfuse ID," which is no longer accurate.📝 Proposed fix
- 1. Validate dataset exists and has Langfuse ID + 1. Validate dataset exists and is backed by Langfuse or object storageAlso applies to: 266-270
🤖 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 `@backend/app/services/evaluations/evaluation.py` at line 212, The docstring for the evaluation start flow is stale because the validation in `start` / `evaluation.py` now accepts either `langfuse_dataset_id` or `object_store_url`, not just a Langfuse ID. Update the docstring text near the evaluation run entry point to match the actual check performed in the dataset validation branch, and ensure any wording around dataset requirements reflects the new “either/or” condition.backend/app/crud/evaluations/processing.py (1)
1198-1279: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
langfuse.flush()called unconditionally afterget_tracing_clientcan returnNone.
get_tracing_client(Line 1198) may returnNonewhen tracing is disabled for a project. The laterlangfuse.flush()call (unguarded) will then raiseAttributeError, silently swallowed by the broadexcept Exception, producing a misleading"Langfuse flush failed"warning on every cron poll for every tracing-off project.🐛 Proposed fix
- try: - langfuse.flush() - except Exception as flush_err: - logger.warning( - f"[poll_all_pending_evaluations] Langfuse flush failed | project_id={project_id} | {flush_err}" - ) + if langfuse is not None: + try: + langfuse.flush() + except Exception as flush_err: + logger.warning( + f"[poll_all_pending_evaluations] Langfuse flush failed | project_id={project_id} | {flush_err}" + )🤖 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 `@backend/app/crud/evaluations/processing.py` around lines 1198 - 1279, The unconditional langfuse.flush() call in poll_all_pending_evaluations can run when get_tracing_client() returns None for tracing-disabled projects, causing a misleading warning. Guard the flush block with a None check on the langfuse client, or skip cleanup entirely when get_tracing_client() returns no client, so only valid Langfuse instances are flushed and the broad exception handler is not triggered unnecessarily.
🧹 Nitpick comments (5)
backend/app/crud/project.py (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace deprecated
typing.List.Ruff (UP035) flags this; use the builtin
listfor consistency with thedict[str, Any]generics already used in this file.🔧 Suggested fix
-from typing import Any, List, Optional +from typing import Any, Optional(Replace any
List[...]usages elsewhere in the file withlist[...].)🤖 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 `@backend/app/crud/project.py` at line 2, Replace the deprecated typing.List import and any List[...] annotations in project.py with the builtin list[...] form to match the existing modern generics style in this module. Update the import list at the top of the file and check any type hints in functions or helpers within the project CRUD code that still reference List, keeping the behavior unchanged while using list consistently.Source: Linters/SAST tools
backend/app/models/project.py (1)
80-92: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winModel's
settingscolumn doesn't declareserver_default, unlike the migration.The migration (071) sets
server_default=sa.text("'{}'::jsonb"), but the SQLAlchemyColumnhere only hasnullable=Falsewith a Python-leveldefault_factory=dict. Since SQLModel'sdefault_factoryisn't visible to Alembic, this mismatch can cause a futurealembic revision --autogenerateto produce a spurious diff attempting to add/remove the server default.🔧 Align model column with migration's server default
settings: dict[str, Any] = Field( default_factory=dict, sa_column=Column( JSONB, nullable=False, + server_default=sa.text("'{}'::jsonb"), comment=( "Project-level settings (JSONB). Keys: 'tracing' (bool) — " "Langfuse tracing opt-in, off by default to conserve Langfuse " "rate-limit/credit budget. Gates tracing for both the response " "path and evaluations (which fall back to cosine-only scoring)." ), ), )Requires importing
sqlalchemy as sa(orfrom sqlalchemy import text) in this file.🤖 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 `@backend/app/models/project.py` around lines 80 - 92, The Project.settings Column is missing the database-level server_default that exists in migration 071, which can cause Alembic autogenerate drift. Update the settings field in project.py so the SQLModel sa_column includes the same JSONB server default as the migration, and add the needed SQLAlchemy import in that model file. Keep the existing default_factory=dict, but make the Column definition in Project.settings match the migration exactly so Alembic sees no schema mismatch.backend/app/utils.py (1)
390-424: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract repeated credential-key literals into a constant.
"public_key","secret_key","host"are duplicated between theall()membership check and the subsequent dict indexing. As per coding guidelines,"Do not use magic values in code; extract repeated literals into constants, Enum members, or settings."♻️ Proposed refactor
+_TRACING_CREDENTIAL_KEYS = ("public_key", "secret_key", "host") + def get_tracing_client( session: Session, org_id: int, project_id: int ) -> Langfuse | None: """Return the Langfuse client when the project opted into tracing, else None (never raises), so evaluations degrade to cosine-only instead of failing.""" credentials = get_tracing_credential( session=session, org_id=org_id, project_id=project_id, ) if not credentials or not all( - key in credentials for key in ["public_key", "secret_key", "host"] + key in credentials for key in _TRACING_CREDENTIAL_KEYS ):🤖 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 `@backend/app/utils.py` around lines 390 - 424, The credential key strings used in get_tracing_client are duplicated and should be centralized. Extract the repeated "public_key", "secret_key", and "host" literals into a single constant near get_tracing_client (or a shared module-level constant) and use it both in the all() validation and the Langfuse constructor indexing to remove magic values and keep the keys consistent.Source: Coding guidelines
backend/app/crud/evaluations/embeddings.py (1)
85-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale docstring reason. The
missing_trace_idskip branch was removed (items now fall back toitem_idasref), soskippedcan only ever carryempty_output/empty_ground_truth. Update the docstring to dropmissing_trace_id.📝 Proposed docstring fix
- - skipped is a list of {item_id, trace_id, reason} for items that - cannot be embedded (reason: empty_output / empty_ground_truth / - missing_trace_id). Used to flag unscoreable items downstream. + - skipped is a list of {item_id, trace_id, reason} for items that + cannot be embedded (reason: empty_output / empty_ground_truth). + Used to flag unscoreable items downstream.🤖 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 `@backend/app/crud/evaluations/embeddings.py` around lines 85 - 87, Update the docstring in the embeddings CRUD logic to match the current skip behavior: the `skipped` list described near the embeddings evaluation flow should no longer mention `missing_trace_id`, since that branch was removed and items now fall back to `item_id` as `ref`; keep the description aligned with the actual reasons emitted by the embedding path (only `empty_output` and `empty_ground_truth`) so the documentation matches the behavior in the relevant `skipped` handling code.backend/app/services/evaluations/fast.py (1)
57-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSync stale comments with the relaxed backing check. Line 89 now accepts a Langfuse or object-store backing, but the docstring step 1 ("Dataset exists and has a Langfuse id") and the inline comment on Line 74 ("must exist + have a Langfuse id") still describe the old Langfuse-only requirement.
Also applies to: 74-74
🤖 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 `@backend/app/services/evaluations/fast.py` around lines 57 - 58, The validation comments in fast evaluation flow are stale and still describe a Langfuse-only requirement, while the backing check now allows Langfuse or object-store. Update the docstring in the fast evaluation service and the inline comment near the dataset validation logic to match the current behavior, using the relevant symbols around the dataset validation path in the fast.py service.
🤖 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 `@backend/app/api/routes/project.py`:
- Around line 90-109: `update_project_settings_route` is missing an explicit
return annotation even though it returns `APIResponse.success_response(project)`
and the route is already tied to `APIResponse[ProjectPublic]`. Add the
appropriate return type hint to the `update_project_settings_route` signature so
the function documents its response type consistently with the rest of the typed
parameters and route contract.
In `@backend/app/crud/credentials.py`:
- Around line 171-197: The `get_tracing_credential` helper currently casts
`project_id` with `int(project_id)` before any validation, so missing or
non-numeric values from `threads.py` can raise and turn tracing into a 500.
Update `get_tracing_credential` to validate or safely cast `project_id` first,
returning `None` when it is absent or invalid, and then continue to the existing
`get_project_by_id` and `get_provider_credential` flow only for valid IDs.
In `@backend/app/crud/evaluations/batch.py`:
- Around line 99-129: The batch item loader in batch.py is re-deciding the
dataset backing at process time instead of reusing the build-time choice, which
can change item IDs and break custom_id matching. Update the eval_run flow so
the selected source is persisted during build (or stored on the run) and have
the item-loading logic in the dataset-fetch path use that saved source instead
of checking tracing/Langfuse again. Make sure the code that calls
fetch_dataset_items and download_csv_from_object_store relies on the same
backing decision as the build step.
In `@backend/app/crud/project.py`:
- Around line 50-74: The update_project_settings flow can lose concurrent
changes because it reads and rewrites project.settings without any concurrency
protection. Fix this by making the read-modify-write in
get_project_by_id/update_project_settings atomic, either by locking the Project
row during fetch (so only one updater can merge settings at a time) or by adding
optimistic version checking before commit. Keep the existing merge behavior and
logging, but ensure the Project settings update cannot silently overwrite
another request’s patch.
---
Outside diff comments:
In `@backend/app/crud/evaluations/processing.py`:
- Around line 1198-1279: The unconditional langfuse.flush() call in
poll_all_pending_evaluations can run when get_tracing_client() returns None for
tracing-disabled projects, causing a misleading warning. Guard the flush block
with a None check on the langfuse client, or skip cleanup entirely when
get_tracing_client() returns no client, so only valid Langfuse instances are
flushed and the broad exception handler is not triggered unnecessarily.
In `@backend/app/services/evaluations/evaluation.py`:
- Line 212: The docstring for the evaluation start flow is stale because the
validation in `start` / `evaluation.py` now accepts either `langfuse_dataset_id`
or `object_store_url`, not just a Langfuse ID. Update the docstring text near
the evaluation run entry point to match the actual check performed in the
dataset validation branch, and ensure any wording around dataset requirements
reflects the new “either/or” condition.
---
Nitpick comments:
In `@backend/app/crud/evaluations/embeddings.py`:
- Around line 85-87: Update the docstring in the embeddings CRUD logic to match
the current skip behavior: the `skipped` list described near the embeddings
evaluation flow should no longer mention `missing_trace_id`, since that branch
was removed and items now fall back to `item_id` as `ref`; keep the description
aligned with the actual reasons emitted by the embedding path (only
`empty_output` and `empty_ground_truth`) so the documentation matches the
behavior in the relevant `skipped` handling code.
In `@backend/app/crud/project.py`:
- Line 2: Replace the deprecated typing.List import and any List[...]
annotations in project.py with the builtin list[...] form to match the existing
modern generics style in this module. Update the import list at the top of the
file and check any type hints in functions or helpers within the project CRUD
code that still reference List, keeping the behavior unchanged while using list
consistently.
In `@backend/app/models/project.py`:
- Around line 80-92: The Project.settings Column is missing the database-level
server_default that exists in migration 071, which can cause Alembic
autogenerate drift. Update the settings field in project.py so the SQLModel
sa_column includes the same JSONB server default as the migration, and add the
needed SQLAlchemy import in that model file. Keep the existing
default_factory=dict, but make the Column definition in Project.settings match
the migration exactly so Alembic sees no schema mismatch.
In `@backend/app/services/evaluations/fast.py`:
- Around line 57-58: The validation comments in fast evaluation flow are stale
and still describe a Langfuse-only requirement, while the backing check now
allows Langfuse or object-store. Update the docstring in the fast evaluation
service and the inline comment near the dataset validation logic to match the
current behavior, using the relevant symbols around the dataset validation path
in the fast.py service.
In `@backend/app/utils.py`:
- Around line 390-424: The credential key strings used in get_tracing_client are
duplicated and should be centralized. Extract the repeated "public_key",
"secret_key", and "host" literals into a single constant near get_tracing_client
(or a shared module-level constant) and use it both in the all() validation and
the Langfuse constructor indexing to remove magic values and keep the keys
consistent.
🪄 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: f088dce3-df91-4d54-91aa-aec4534119c1
📒 Files selected for processing (29)
backend/app/alembic/versions/071_add_settings_to_project.pybackend/app/api/docs/projects/update_settings.mdbackend/app/api/routes/project.pybackend/app/api/routes/responses.pybackend/app/api/routes/threads.pybackend/app/crud/__init__.pybackend/app/crud/credentials.pybackend/app/crud/evaluations/batch.pybackend/app/crud/evaluations/core.pybackend/app/crud/evaluations/embeddings.pybackend/app/crud/evaluations/fast.pybackend/app/crud/evaluations/langfuse.pybackend/app/crud/evaluations/processing.pybackend/app/crud/project.pybackend/app/models/__init__.pybackend/app/models/project.pybackend/app/services/evaluations/batch_job.pybackend/app/services/evaluations/dataset.pybackend/app/services/evaluations/evaluation.pybackend/app/services/evaluations/fast.pybackend/app/services/llm/jobs.pybackend/app/services/response/response.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/api/routes/test_evaluation_fast.pybackend/app/tests/crud/evaluations/test_batch.pybackend/app/tests/crud/evaluations/test_embeddings.pybackend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/services/evaluations/test_evaluation_service_s3.pybackend/app/utils.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/crud/evaluations/processing.py (1)
342-369: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring: items are no longer skipped without a
trace_id.The docstring says: "Items without a trace_id are skipped." But the fallback
ref = trace_id_mapping.get(item_id) or item_idmeans an item without a trace_id still gets a record (keyed byitem_id); it's only skipped ifitem_idis also falsy. Update the docstring to reflect the fallback behavior so future readers don't misread the semantics ofrefhere (which is central to the new tracing opt-out feature).📝 Proposed docstring fix
""" Build per-trace records (Q&A keyed by Langfuse trace_id, no scores yet) from parsed evaluation results. Persisted at the response stage so the embedding-completion step can attach cosine scores and write a complete trace unit, making cosine display independent of Langfuse. Items without a trace_id - are skipped. + fall back to using item_id as the reference; only items lacking both are + skipped. """🤖 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 `@backend/app/crud/evaluations/processing.py` around lines 342 - 369, Update the docstring for build_trace_skeleton to match the actual fallback behavior of ref: items are not skipped just because trace_id is missing, since trace_id_mapping.get(item_id) or item_id still creates a record keyed by item_id; only falsy item_id values are skipped. Keep the wording aligned with the trace_id_mapping lookup and the trace_id field so the semantics of the tracing opt-out path are clear.
♻️ Duplicate comments (1)
backend/app/crud/evaluations/batch.py (1)
79-119: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSame build-vs-process source inconsistency as flagged previously — not resolved by this refactor.
use_langfuse_clientandload_evaluation_dataset_itemsstill re-derive the Langfuse-vs-object-store decision independently at build time (execute_evaluation_batch_submission) and at process time (check_and_process_evaluation→process_completed_evaluation, run much later via async polling). The decision depends on the livelangfuseclient (itself derived from the current, mutablesettings["tracing"]project flag), not on anything persisted oneval_run.If tracing is toggled between submission and completion, the run submits batch requests keyed by Langfuse item IDs (e.g.
lf_1) but later reloads items keyed byitem_{row}_{dup}(or vice versa) — breakingcustom_idmatching in the response-parsing stage. This is the exact class of issue raised in a previous review round; it's been renamed (reconcile_tracing_client→use_langfuse_client) but not architecturally fixed. Persist the resolved source (or resolved langfuse-usage flag) oneval_runat build time and reuse it at process time instead of re-deriving from current settings.🤖 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 `@backend/app/crud/evaluations/batch.py` around lines 79 - 119, The Langfuse-vs-object-store source choice is still being recomputed separately in use_langfuse_client and load_evaluation_dataset_items, which can change between submission and later processing. Persist the resolved tracing source or a Langfuse-enabled flag on EvaluationRun when execute_evaluation_batch_submission builds the run, then have check_and_process_evaluation/process_completed_evaluation reuse that stored decision instead of consulting the current settings or re-deriving via use_langfuse_client/get_dataset_by_id.
🧹 Nitpick comments (2)
backend/app/crud/evaluations/batch.py (2)
122-154: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo upper bound on
duplication_factor.
max(1, int((dataset.dataset_metadata or {}).get("duplication_factor", 1)))only floors the value; an unexpectedly largeduplication_factorindataset_metadatawill multiplyoriginal_itemsaccordingly with no cap, which could blow up batch size/memory for a single run.🤖 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 `@backend/app/crud/evaluations/batch.py` around lines 122 - 154, The duplication handling in _load_items_from_object_store only enforces a minimum via duplication_factor and can still expand original_items without bound. Add an upper limit or validation when reading duplication_factor from dataset.dataset_metadata, and clamp or reject excessively large values before the nested loop builds the items list. Keep the fix localized to _load_items_from_object_store so the batch sizing remains safe.
122-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
dataset: Anyparameter can be narrowed.
datasethere is the object returned byget_dataset_by_id, presumably a concrete model type rather thanAny. As per coding guidelines, "Use type hints on every function parameter and return value;-> Anyis not acceptable unless the type can be narrowed" — the same spirit applies to parameter types.🤖 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 `@backend/app/crud/evaluations/batch.py` around lines 122 - 124, The _load_items_from_object_store function currently accepts dataset as Any, but it should use a narrower concrete type matching the object returned by get_dataset_by_id. Update the dataset parameter in _load_items_from_object_store to the specific dataset model/class used throughout this module, and keep the session parameter and return type explicit so the signature reflects the actual data shape instead of Any.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.
Outside diff comments:
In `@backend/app/crud/evaluations/processing.py`:
- Around line 342-369: Update the docstring for build_trace_skeleton to match
the actual fallback behavior of ref: items are not skipped just because trace_id
is missing, since trace_id_mapping.get(item_id) or item_id still creates a
record keyed by item_id; only falsy item_id values are skipped. Keep the wording
aligned with the trace_id_mapping lookup and the trace_id field so the semantics
of the tracing opt-out path are clear.
---
Duplicate comments:
In `@backend/app/crud/evaluations/batch.py`:
- Around line 79-119: The Langfuse-vs-object-store source choice is still being
recomputed separately in use_langfuse_client and load_evaluation_dataset_items,
which can change between submission and later processing. Persist the resolved
tracing source or a Langfuse-enabled flag on EvaluationRun when
execute_evaluation_batch_submission builds the run, then have
check_and_process_evaluation/process_completed_evaluation reuse that stored
decision instead of consulting the current settings or re-deriving via
use_langfuse_client/get_dataset_by_id.
---
Nitpick comments:
In `@backend/app/crud/evaluations/batch.py`:
- Around line 122-154: The duplication handling in _load_items_from_object_store
only enforces a minimum via duplication_factor and can still expand
original_items without bound. Add an upper limit or validation when reading
duplication_factor from dataset.dataset_metadata, and clamp or reject
excessively large values before the nested loop builds the items list. Keep the
fix localized to _load_items_from_object_store so the batch sizing remains safe.
- Around line 122-124: The _load_items_from_object_store function currently
accepts dataset as Any, but it should use a narrower concrete type matching the
object returned by get_dataset_by_id. Update the dataset parameter in
_load_items_from_object_store to the specific dataset model/class used
throughout this module, and keep the session parameter and return type explicit
so the signature reflects the actual data shape instead of Any.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7685ad7-ca8b-4f4c-a2e1-6bb1988eebb4
📒 Files selected for processing (9)
backend/app/api/routes/project.pybackend/app/crud/credentials.pybackend/app/crud/evaluations/batch.pybackend/app/crud/evaluations/fast.pybackend/app/crud/evaluations/processing.pybackend/app/crud/project.pybackend/app/models/project.pybackend/app/services/evaluations/batch_job.pybackend/app/tests/crud/evaluations/test_batch.py
💤 Files with no reviewable changes (1)
- backend/app/models/project.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/crud/project.py
- backend/app/crud/credentials.py
- backend/app/api/routes/project.py
vprashrex
changed the title
vprashrex
changed the title
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
1 participant
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.
Issue: #930
Summary
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes