Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 21 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds task-tag persistence and error mapping in the database layer, exposes a Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 new TaskFactory/DatasetFactory fixtures always insert with a fixed default ID (42_000), which can easily lead to primary key collisions if they are called more than once per test; consider generating unique IDs (e.g. via a counter or random range) to make the fixture safer to reuse.
- The
TaskNotFoundErrormapping intag_taskuses a hard-coded numeric error code (472) which is then duplicated in tests; centralizing this code in a shared constant would reduce the risk of future mismatches.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new TaskFactory/DatasetFactory fixtures always insert with a fixed default ID (42_000), which can easily lead to primary key collisions if they are called more than once per test; consider generating unique IDs (e.g. via a counter or random range) to make the fixture safer to reuse.
- The `TaskNotFoundError` mapping in `tag_task` uses a hard-coded numeric error code (472) which is then duplicated in tests; centralizing this code in a shared constant would reduce the risk of future mismatches.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #350 +/- ##
==========================================
+ Coverage 95.00% 95.09% +0.09%
==========================================
Files 72 74 +2
Lines 3580 3693 +113
Branches 243 245 +2
==========================================
+ Hits 3401 3512 +111
- Misses 115 116 +1
- Partials 64 65 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/routers/openml/tag_test_helper.py (2)
50-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant status check.
already_tagged(lines 27-30) is onlyTruewhenphp_response.status_code == INTERNAL_SERVER_ERROR, so the extra status comparison on line 51 is always satisfied and can be dropped for clarity.♻️ Simplify the condition
- if php_response.status_code == HTTPStatus.INTERNAL_SERVER_ERROR and already_tagged: + if already_tagged:🤖 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/routers/openml/tag_test_helper.py` around lines 50 - 51, The condition in the tag conflict handling helper is redundant because already_tagged is only true when php_response.status_code is INTERNAL_SERVER_ERROR. Simplify the check in the tag_test_helper flow by removing the extra status comparison and relying on already_tagged alone, keeping the logic around the php_response handling and the helper’s conflict detection clear and easier to read.
32-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment typo: "taskbase".
These comments read "persist this change to the taskbase" / "committed to the taskbase", which looks like a stray word (likely "database").
📝 Suggested wording
- # undo the tag, because we don't want to persist this change to the taskbase - # Sometimes a change is already committed to the taskbase even if an error occurs. + # undo the tag, because we don't want to persist this change to the database + # Sometimes a change is already committed to the database even if an error occurs.🤖 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/routers/openml/tag_test_helper.py` around lines 32 - 33, The comments in tag_test_helper.py use the stray word “taskbase” in the undo/persistence note. Update those nearby comments to use the intended term consistently, likely “database,” and keep the wording aligned with the surrounding logic in the tag helper comments.tests/routers/openml/task_tag_test.py (1)
90-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParity test parametrizes
task_idwith dataset-specific constants.
constants.SOME_DEACTIVATED_DATASET_IDandconstants.DATASET_ID_THAT_DOES_NOT_EXISTare dataset identifiers used here astask_idvalues. They function as arbitrary integers for PHP/Python parity, but the names imply dataset semantics and obscure the intent (existing vs. non-existing task). Consider task-oriented constants or inline literals with explanatory ids.🤖 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/routers/openml/task_tag_test.py` around lines 90 - 98, The parity test in task_tag_test.py is using dataset-named constants as task_id inputs, which obscures the intent of the parametrization. Update the task_id cases in the parametrized test near the task_id fixture to use task-oriented identifiers or explicit integer literals with clear naming/comments, so it is obvious which values represent existing versus non-existing tasks. Keep the change localized to the test data setup around the pytest.mark.parametrize block.
🤖 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 `@tests/routers/openml/tag_test_helper.py`:
- Around line 50-51: The condition in the tag conflict handling helper is
redundant because already_tagged is only true when php_response.status_code is
INTERNAL_SERVER_ERROR. Simplify the check in the tag_test_helper flow by
removing the extra status comparison and relying on already_tagged alone,
keeping the logic around the php_response handling and the helper’s conflict
detection clear and easier to read.
- Around line 32-33: The comments in tag_test_helper.py use the stray word
“taskbase” in the undo/persistence note. Update those nearby comments to use the
intended term consistently, likely “database,” and keep the wording aligned with
the surrounding logic in the tag helper comments.
In `@tests/routers/openml/task_tag_test.py`:
- Around line 90-98: The parity test in task_tag_test.py is using dataset-named
constants as task_id inputs, which obscures the intent of the parametrization.
Update the task_id cases in the parametrized test near the task_id fixture to
use task-oriented identifiers or explicit integer literals with clear
naming/comments, so it is obvious which values represent existing versus
non-existing tasks. Keep the change localized to the test data setup around the
pytest.mark.parametrize block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ad0693cf-3d9d-4aaf-b625-05ed55d0c9eb
📒 Files selected for processing (6)
src/database/tasks.pysrc/routers/openml/tasks.pytests/conftest.pytests/routers/openml/dataset_tag_test.pytests/routers/openml/tag_test_helper.pytests/routers/openml/task_tag_test.py
1 participant
Closes #26.
Description
Adds an endpoint for tagging a task.
Also refactors the
/datasets/tagendpoint tests to be less dependent on database state.Checklist
Please check all that apply. You can mark items as N/A if they don't apply to your change.
Always:
Required for code changes:
If applicable:
/docs)Extra context:
nb. If tests pass locally but fail on CI, please try to investigate the cause. If you are unable to resolve the issue, please share your findings.