Skip to content

refactor setup tag to be similar to dataset and task#352

Merged
PGijsbers merged 1 commit into
mainfrom
update-setup-tag
Jun 30, 2026
Merged

refactor setup tag to be similar to dataset and task#352
PGijsbers merged 1 commit into
mainfrom
update-setup-tag

Conversation

@PGijsbers

Copy link
Copy Markdown
Contributor

Description

Refactoring the setup tag function to be more similar to the way it's implemented for datasets and tasks for consistency.

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The setup tag database function now wraps its INSERT in a try/except IntegrityError block that reads the underlying DB error code and raises ForeignKeyConstraintError or DuplicatePrimaryKeyError accordingly. The tag_setup router endpoint removes its previous async pre-fetch and Python-side matching logic, instead catching these typed exceptions to produce SetupNotFoundError (code 472) or TagAlreadyExistsError. After a successful insert, it queries all tag rows to build the response. Two test assertions are updated to match the new "already tagged with" error message wording.

Possibly related PRs

  • openml/server-api#298: Refactors the same setups_tag_test.py to assert SetupNotFoundError/TagAlreadyExistsError message patterns via direct calls, directly overlapping with this PR's test changes.
  • openml/server-api#322: Applies the same pattern of mapping DB IntegrityError to ForeignKeyConstraintError/DuplicatePrimaryKeyError in a tag insert and updating the corresponding router, but for the datasets resource.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the refactor to setup tagging and its alignment with dataset and task behavior.
Description check ✅ Passed The description is clearly related to the refactor and matches the changes in setup tag handling.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch update-setup-tag

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.17%. Comparing base (b61e974) to head (c80b2af).

Files with missing lines Patch % Lines
src/database/setups.py 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
- Coverage   95.24%   95.17%   -0.07%     
==========================================
  Files          74       74              
  Lines        3699     3710      +11     
  Branches      244      244              
==========================================
+ Hits         3523     3531       +8     
- Misses        113      114       +1     
- Partials       63       65       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/routers/openml/setups.py (1)

62-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant code=472 override. SetupNotFoundError already defines _default_code = 472, so the explicit override is unnecessary and can drift from the default if either changes. Consider dropping it for clarity.

🤖 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/routers/openml/setups.py` at line 62, The SetupNotFoundError raise in the
setup lookup path is redundantly overriding the default status code, which can
drift from the class definition. Update the relevant error path in the setup
handling logic to raise SetupNotFoundError with just the message, relying on the
class’s _default_code in SetupNotFoundError instead of passing code=472
explicitly.
🤖 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.

Nitpick comments:
In `@src/routers/openml/setups.py`:
- Line 62: The SetupNotFoundError raise in the setup lookup path is redundantly
overriding the default status code, which can drift from the class definition.
Update the relevant error path in the setup handling logic to raise
SetupNotFoundError with just the message, relying on the class’s _default_code
in SetupNotFoundError instead of passing code=472 explicitly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 6737c17b-7d65-4ffc-a8ea-bab39afcabc4

📥 Commits

Reviewing files that changed from the base of the PR and between b61e974 and c80b2af.

📒 Files selected for processing (3)
  • src/database/setups.py
  • src/routers/openml/setups.py
  • tests/routers/openml/setups_tag_test.py

@PGijsbers PGijsbers merged commit 8adb636 into main Jun 30, 2026
7 of 9 checks passed
@PGijsbers PGijsbers deleted the update-setup-tag branch June 30, 2026 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant