Setup tag orm#355
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThis PR migrates setup and setup-tag database access from raw SQL over AsyncConnection to SQLAlchemy ORM over AsyncSession. New ORM models Setup and SetupTag are introduced. database.setups functions (get, tag, get_tags) are rewritten, and get_tag/delete_tag are added while untag is removed. The expdb_session dependency no longer opens an explicit transaction. Routers for runs and setups are updated to use expdb_session and Setup.flow_id instead of implementation_id. Corresponding tests are updated to use AsyncSession fixtures. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #355 +/- ##
==========================================
- Coverage 94.51% 94.51% -0.01%
==========================================
Files 76 77 +1
Lines 3775 3792 +17
Branches 248 248
==========================================
+ Hits 3568 3584 +16
- Misses 139 140 +1
Partials 68 68 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 found 1 issue, and left some high level feedback:
- The
expdb_sessiondependency no longer wraps theAsyncSessioninsession.begin(), so ORM operations liketag/delete_tagmay not be committed or wrapped in a transaction; consider reintroducing an explicit transactional context (or an explicitcommit) around these request-scoped sessions. - In
untag_setup,get_tagnow relies on a straight primary-key lookup (tag+entity_id) and no longer performs a case-insensitive match like the previouscasefold()logic, which changes behavior for tags that differ only by case; if case-insensitive semantics are still desired, you may want to preserve or reintroduce a case-insensitive query.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `expdb_session` dependency no longer wraps the `AsyncSession` in `session.begin()`, so ORM operations like `tag`/`delete_tag` may not be committed or wrapped in a transaction; consider reintroducing an explicit transactional context (or an explicit `commit`) around these request-scoped sessions.
- In `untag_setup`, `get_tag` now relies on a straight primary-key lookup (`tag` + `entity_id`) and no longer performs a case-insensitive match like the previous `casefold()` logic, which changes behavior for tags that differ only by case; if case-insensitive semantics are still desired, you may want to preserve or reintroduce a case-insensitive query.
## Individual Comments
### Comment 1
<location path="src/database/setups.py" line_range="62-64" />
<code_context>
- ),
- parameters={"setup_id": setup_id, "tag": tag},
- )
+async def get_tag(setup_id: Identifier, tag: TagString, session: AsyncSession) -> SetupTag | None:
+ """Get the tag `tag` for setup with id `setup_id`."""
+ return await session.get(SetupTag, {"tag": tag, "entity_id": setup_id})
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `session.get` with a dict for a composite primary key is likely incorrect.
`Session.get` only accepts a scalar PK or a tuple of PK values in mapper order for composite keys; dicts aren’t supported and will likely fail at runtime. Since `SetupTag`’s PK appears to be `(entity_id, tag)`, this should use an explicit `select`, e.g.:
```python
stmt = (
select(SetupTag)
.where(SetupTag.entity_id == setup_id)
.where(SetupTag.tag == tag)
)
return (await session.scalars(stmt)).one_or_none()
```
This also keeps the lookup consistent with `get_tags`, which already uses a `select` on `entity_id`.
</issue_to_address>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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/database/setups.py (1)
79-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
IntegrityError→ domain-error mapping logic.This try/except block (checking
e.orig.argsagainst_FOREIGN_KEY_CONSTRAINT_FAILED/_DUPLICATE_ENTRYand re-raising asForeignKeyConstraintError/DuplicatePrimaryKeyError) duplicates the equivalent block insrc/database/tasks.py(tag()at lines 168-176). Consider extracting a shared helper (e.g.,_raise_domain_error_from_integrity_error(e)) to avoid the two copies diverging over time.♻️ Example extraction
def _map_integrity_error(e: IntegrityError) -> None: if e.orig is None: raise e code, msg = e.orig.args if code == _FOREIGN_KEY_CONSTRAINT_FAILED: raise ForeignKeyConstraintError(msg) from e if code == _DUPLICATE_ENTRY: raise DuplicatePrimaryKeyError(msg) from e raise e🤖 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/database/setups.py` around lines 79 - 91, The IntegrityError-to-domain-error translation in SetupTag creation is duplicated and should be centralized. Extract the shared mapping logic from the try/except in the setup tag flow into a helper such as _raise_domain_error_from_integrity_error or _map_integrity_error, and reuse that same helper from both the setups path and the matching tag() logic in tasks so the _FOREIGN_KEY_CONSTRAINT_FAILED and _DUPLICATE_ENTRY handling stays consistent.
🤖 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/database/schema/setups.py`:
- Around line 9-16: The Setup ORM class is inheriting from the reflected-model
bases in the wrong order, which should match the documented pattern used by the
other reflected models. Update Setup to follow the same base order as the
reflected ORM classes referenced in the schema base and sibling tag models,
using the Setup class definition to locate it.
In `@src/routers/openml/runs.py`:
- Around line 100-112: The setup lookup in the `get_run` flow is using
`database.setups.get(run.setup, expdb_session)` inside `asyncio.gather`, which
reuses the same `AsyncConnection` as the other `database.runs`/`database.tasks`
queries. Move the `database.setups.get` call out of the concurrent gather in
this `runs.py` handler, or fetch it through a separate session/connection, while
keeping the other gather calls unchanged.
---
Nitpick comments:
In `@src/database/setups.py`:
- Around line 79-91: The IntegrityError-to-domain-error translation in SetupTag
creation is duplicated and should be centralized. Extract the shared mapping
logic from the try/except in the setup tag flow into a helper such as
_raise_domain_error_from_integrity_error or _map_integrity_error, and reuse that
same helper from both the setups path and the matching tag() logic in tasks so
the _FOREIGN_KEY_CONSTRAINT_FAILED and _DUPLICATE_ENTRY handling stays
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a120ffd5-b4b1-47c7-9a1a-2479857fa99d
📒 Files selected for processing (9)
src/database/schema/setups.pysrc/database/schema/tags.pysrc/database/setups.pysrc/database/tasks.pysrc/routers/dependencies.pysrc/routers/openml/runs.pysrc/routers/openml/setups.pytests/routers/openml/setups_tag_test.pytests/routers/openml/setups_untag_test.py
1 participant
Description
Move towards using ORM for all simple operations: setup tags and fetching the setup.