feat(roles): allow ML Data Manager to run batch ML jobs#1212
Conversation
The RUN_ML_JOB permission was intentionally revoked from the MLDataManager role during initial rollout. This enables it so that project members with the ML Data Manager or Project Manager role can run batch ML jobs, not just superusers. The permission is synced to existing projects on the next migrate (the post_migrate signal calls create_roles_for_project for all projects). Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for antenna-ssec canceled.
|
✅ Deploy Preview for antenna-preview canceled.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
Updates project role permissions so that non-superusers (specifically ML Data Manager, and by inheritance Project Manager) can run batch ML jobs via the existing fine-grained run_ml_job permission.
Changes:
- Grant
Project.Permissions.RUN_ML_JOBto theMLDataManagerrole permissions set. - Remove the now-outdated inline comment indicating ML job running was temporarily revoked.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Verify that MLDataManager and ProjectManager roles can run batch ML jobs via the full role system (create_roles_for_project + role assignment), and that BasicMember cannot. Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ami/main/tests.py (1)
2030-2048: Optional: reduce duplication with a parameterized role matrix.The three run-authorization tests are structurally identical and can be merged into one table-driven test (
subTest) for easier maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/tests.py` around lines 2030 - 2048, Replace the three near-duplicate tests (test_ml_data_manager_can_run_ml_job, test_project_manager_can_run_ml_job, test_basic_member_cannot_run_ml_job) with a single parameterized test that iterates over a role→expected_status matrix; inside the loop use self.client.force_authenticate(role_user), call job = self._create_ml_job() and POST to f"/api/v2/jobs/{job.pk}/run/" and assert the response.status_code equals the expected status, using subTest or pytest.mark.parametrize to keep separate failures distinct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ami/main/tests.py`:
- Around line 2030-2048: Replace the three near-duplicate tests
(test_ml_data_manager_can_run_ml_job, test_project_manager_can_run_ml_job,
test_basic_member_cannot_run_ml_job) with a single parameterized test that
iterates over a role→expected_status matrix; inside the loop use
self.client.force_authenticate(role_user), call job = self._create_ml_job() and
POST to f"/api/v2/jobs/{job.pk}/run/" and assert the response.status_code equals
the expected status, using subTest or pytest.mark.parametrize to keep separate
failures distinct.
The TestRolePermissions permission map for project_manager now expects run=True for ML jobs (matching the MLDataManager role change), and removes retry/cancel entries that aren't surfaced as separate permissions in the API response (they share the run_ml_job codename). Co-Authored-By: Claude <noreply@anthropic.com>
Address CodeRabbit review feedback: merge three near-identical role authorization tests into a single table-driven test using subTest. Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude says: CodeRabbit's nitpick about consolidating the three duplicate test methods into a parameterized subTest has been addressed in commit f904af4. |
Summary
run_ml_jobpermission to the ML Data Manager role, which also flows to Project Manager via role inheritancemigrate(thepost_migratesignal recreates role groups for all projects)Test plan
TestMLDataManagerCanRunBatchMLJobtests verify role-based permission flow end-to-end:runpermission reflected in job detail response for MLDataManagerTestFineGrainedJobRunPermissiontests still passTestRunSingleImageJobPermissiontests still passami.users.testsrole/membership tests passSummary by CodeRabbit
Bug Fixes
Tests