Add GET /runs/trace/{run_id} endpoint#267
Add GET /runs/trace/{run_id} endpoint#267saathviksheerla wants to merge 5 commits intoopenml:mainfrom
GET /runs/trace/{run_id} endpoint#267Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds database helpers in src/database/runs.py to fetch a run and its ordered trace iterations. Adds Pydantic schemas in src/schemas/runs.py for trace responses. Introduces a FastAPI router at /runs/trace/{run_id} in src/routers/openml/runs.py that verifies run existence, retrieves ordered trace rows (ordered by repeat, fold, iteration), returns a structured RunTraceResponse on success, and returns HTTP 412 with distinct codes for missing run or missing trace. Registers the router in src/main.py. Adds tests in tests/routers/openml/runs_test.py covering success, ordering, missing run, missing trace, and invalid run_id cases. 🚥 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)
📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
expdbdependency inget_run_tracedoesn't need a default ofNone—dropping the default makes the intent clearer that it must always be provided by FastAPI's dependency injection. - In
TraceIteration, consider constrainingselectedto aLiteral["true", "false"]to encode the PHP-compatible value space directly in the schema and catch accidental deviations early.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `expdb` dependency in `get_run_trace` doesn't need a default of `None`—dropping the default makes the intent clearer that it must always be provided by FastAPI's dependency injection.
- In `TraceIteration`, consider constraining `selected` to a `Literal["true", "false"]` to encode the PHP-compatible value space directly in the schema and catch accidental deviations early.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/routers/openml/runs.py (1)
35-47: Normalizesetup_string,evaluation, andselectedto match the explicit type-casting of other fields.The code explicitly normalizes
run_id,repeat,fold, anditerationwithstr(), but forwardssetup_string,evaluation, andselecteddirectly from the SQL row into a schema requiringstr. While tests validate these fields are already strings andselectedis "true" or "false", this inconsistency creates a fragility point: if the database ever stores aNULLvalue or non-string type for these fields, Pydantic model construction will fail without a clear error message.Add
str()calls to these three fields at lines 44-46 to match the pattern used for the other iteration fields and ensure consistent defensive programming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routers/openml/runs.py` around lines 35 - 47, The TraceIteration construction in RunTraceResponse currently casts run_id/repeat/fold/iteration to str but passes setup_string, evaluation, and selected directly; update the TraceIteration creation (inside RunTraceResponse -> RunTrace -> trace_iteration) to wrap setup_string, evaluation, and selected with str() as well so all iteration fields are normalized (e.g., use str(row.setup_string), str(row.evaluation), str(row.selected)) to avoid Pydantic errors from NULL/non-string DB values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/routers/openml/runs.py`:
- Around line 35-47: The TraceIteration construction in RunTraceResponse
currently casts run_id/repeat/fold/iteration to str but passes setup_string,
evaluation, and selected directly; update the TraceIteration creation (inside
RunTraceResponse -> RunTrace -> trace_iteration) to wrap setup_string,
evaluation, and selected with str() as well so all iteration fields are
normalized (e.g., use str(row.setup_string), str(row.evaluation),
str(row.selected)) to avoid Pydantic errors from NULL/non-string DB values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7a66e163-c68e-4081-935d-0ad5753181e2
📒 Files selected for processing (5)
src/database/runs.pysrc/main.pysrc/routers/openml/runs.pysrc/schemas/runs.pytests/routers/openml/runs_test.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #267 +/- ##
=======================================
Coverage ? 55.04%
=======================================
Files ? 37
Lines ? 1477
Branches ? 118
=======================================
Hits ? 813
Misses ? 662
Partials ? 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routers/openml/runs.py`:
- Around line 40-47: Convert the `selected` value to a string when constructing
the TraceIteration so it matches the other iteration fields and the Pydantic
schema: change the construction in the TraceIteration call that currently passes
selected=row.selected to use selected=str(row.selected) (locate the
TraceIteration instantiation that sets repeat=str(row.repeat),
fold=str(row.fold), iteration=str(row.iteration)) so `selected` becomes a string
literal "true"/"false" as expected by the model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b3a49664-e599-42cd-b1f4-f36558498629
📒 Files selected for processing (2)
src/routers/openml/runs.pysrc/schemas/runs.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/schemas/runs.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routers/openml/runs.py`:
- Line 51: Normalize the selected value explicitly instead of relying on
truthiness: replace the ternary that uses row.selected (the selected key in the
serialized output) with an explicit normalization check that handles booleans,
integers, enums, and strings (for example, treat values as true only if they
match True, 1, "1", "true", "t", or "yes" case-insensitively); update the
serialization that sets selected="true"/"false" to use that normalization logic
(or a small helper like normalize_selected(value)) so string values like "false"
no longer serialize as "true".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1415a658-924d-4dbd-a450-2656165973f4
📒 Files selected for processing (1)
src/routers/openml/runs.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routers/openml/runs.py`:
- Around line 45-51: When constructing the TraceIteration instance, explicitly
cast the database value for evaluation to a string instead of forwarding the raw
value; update the TraceIteration(...) call (the evaluation argument alongside
repeat, fold, iteration) to use str(row.evaluation) so TraceIteration.evaluation
is always a string and matches the model/tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0101722f-6ed4-4983-a863-5d2df0d9f22d
📒 Files selected for processing (1)
src/routers/openml/runs.py
|
@PGijsbers ready for your review. |
saathviksheerla
force-pushed
the
feat/get-run-trace-endpoint
branch
from
53a1e33 to
6b876f3
Compare
1 participant
Closes #40
Implements the
GET /runs/trace/{run_id}endpoint, migrating behavior from the PHP API.Changes
src/database/runs.py— SQL queries for run existence check and trace fetchsrc/schemas/runs.py— Pydantic models matching PHP response structuresrc/routers/openml/runs.py— FastAPI endpointsrc/main.py— router registrationtests/routers/openml/runs_test.py— 6 tests covering happy path, ordering, and error casesBehavior