Conversation
📝 WalkthroughWalkthroughThe pull request adds comprehensive support for handling canceled DICOM jobs and tasks. It extends the job post-processing logic to recognize canceled states, adds an exception handler for task cancellation events, and includes tests validating the new cancellation workflow for both job and task entities. Changes
Sequence DiagramsequenceDiagram
participant Context as Abort Context
participant Task as process_dicom_task
participant Processor as ExampleProcessor
participant DicomTask as DicomTask (DB)
participant Job as DicomJob (DB)
Context->>Task: should_abort() → True
Task->>Processor: process()
Note over Processor: Blocking operation (sleep)
Task->>Task: Check abort signal
Task->>Processor: Raise CancelledError
Processor-->>Task: futures.CancelledError
Task->>DicomTask: Set status = CANCELED
Task->>DicomTask: Set message = "Task was canceled."
Task->>DicomTask: Commit to DB
Note over Task: Task cancellation logged
Job->>Job: post_process()
Job->>DicomTask: Query all tasks
DicomTask-->>Job: [CANCELED, CANCELED, ...]
Job->>Job: No success/warning/failure found
Job->>Job: At least one CANCELED exists
Job->>Job: Set status = CANCELED
Job->>Job: Set message = "All tasks were canceled."
Job->>Job: Set end timestamp
Job->>Job: Commit to DB
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where attempting to kill a task in the admin view would result in an error rather than a proper cancellation. The changes ensure that when a task is canceled, it and its associated job are correctly marked with a 'CANCELED' status, providing a more accurate and functional cancellation mechanism within the system. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request implements a comprehensive cancellation mechanism for DICOM jobs and tasks. This includes handling futures.CancelledError to mark individual tasks as CANCELED, and updating job statuses to CANCELED when tasks are canceled or the job is in a CANCELING state. New tests were added to validate these cancellation flows. A review comment suggests an improvement in adit/core/models.py to prevent an unnecessary database write when a job is already in the CANCELED status.
| if self.status in [DicomJob.Status.CANCELING, DicomJob.Status.CANCELED]: | ||
| self.status = DicomJob.Status.CANCELED | ||
| self.save() | ||
| return False |
There was a problem hiding this comment.
This condition correctly includes DicomJob.Status.CANCELED, but it leads to an unnecessary database write if the job status is already CANCELED. You can avoid this by checking the status before saving.
| if self.status in [DicomJob.Status.CANCELING, DicomJob.Status.CANCELED]: | |
| self.status = DicomJob.Status.CANCELED | |
| self.save() | |
| return False | |
| if self.status in (DicomJob.Status.CANCELING, DicomJob.Status.CANCELED): | |
| if self.status == DicomJob.Status.CANCELING: | |
| self.status = DicomJob.Status.CANCELED | |
| self.save() | |
| return False |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
adit/core/tests/test_tasks.py (1)
184-194: Consider adding a properjobattribute toFakeContextfor future-proofing.The
FakeContext.job = object()works for this test since theCancelledErrorpath doesn't accesscontext.job.attempts. However, if the code evolves to access job attributes in the cancellation path, this test would fail unexpectedly.💡 Optional: Add a minimal job mock
class FakeContext: def __init__(self): - self.job = object() + class FakeJob: + attempts = 0 + self.job = FakeJob() def should_abort(self): return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adit/core/tests/test_tasks.py` around lines 184 - 194, FakeContext currently sets job = object(), which may break if cancellation paths read job attributes; update FakeContext used in the process_dicom_task test to provide a minimal job mock with the attributes the code might access (e.g., an attempts integer and any other small fields expected), so replace the plain object with a lightweight stub (e.g., an instance or simple namespace) that defines .attempts (and any other referenced properties) to avoid future brittle tests referencing JobContext, ExampleTransferTask, or process_dicom_task.adit/core/models.py (1)
303-305: Add tests to document behavior for mixed canceled/completed task scenarios.The code correctly prioritizes completed task outcomes (SUCCESS/FAILURE/WARNING) over CANCELED status due to the elif chain. When a job has mixed task statuses (e.g., 1 SUCCESS + 1 CANCELED), the job reflects the completed task's outcome. Current tests cover all-CANCELED scenarios but lack coverage for mixed scenarios; adding tests for 1 SUCCESS + 1 CANCELED and 1 FAILURE + 1 CANCELED cases would explicitly document this expected behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adit/core/models.py` around lines 303 - 305, Add unit tests that assert mixed-task behavior: create a DicomJob with two tasks where one task is DicomJob.Status.SUCCESS and the other is DicomJob.Status.CANCELED, and another test with one DicomJob.Status.FAILURE plus one DicomJob.Status.CANCELED; run whatever routine sets the job status (the logic that sets self.status using DicomJob.Status.*) and assert the job.status reflects SUCCESS in the first test and FAILURE in the second (not CANCELED), and that job.message is not "All tasks were canceled." to document the precedence of completed outcomes over CANCELED.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@adit/core/models.py`:
- Around line 303-305: Add unit tests that assert mixed-task behavior: create a
DicomJob with two tasks where one task is DicomJob.Status.SUCCESS and the other
is DicomJob.Status.CANCELED, and another test with one DicomJob.Status.FAILURE
plus one DicomJob.Status.CANCELED; run whatever routine sets the job status (the
logic that sets self.status using DicomJob.Status.*) and assert the job.status
reflects SUCCESS in the first test and FAILURE in the second (not CANCELED), and
that job.message is not "All tasks were canceled." to document the precedence of
completed outcomes over CANCELED.
In `@adit/core/tests/test_tasks.py`:
- Around line 184-194: FakeContext currently sets job = object(), which may
break if cancellation paths read job attributes; update FakeContext used in the
process_dicom_task test to provide a minimal job mock with the attributes the
code might access (e.g., an attempts integer and any other small fields
expected), so replace the plain object with a lightweight stub (e.g., an
instance or simple namespace) that defines .attempts (and any other referenced
properties) to avoid future brittle tests referencing JobContext,
ExampleTransferTask, or process_dicom_task.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e4c11bf-260a-4954-ab38-9b5e08aa13ce
📒 Files selected for processing (4)
adit/core/models.pyadit/core/tasks.pyadit/core/tests/test_models.pyadit/core/tests/test_tasks.py
2 participants
In the admin view, if clicking on the "kill task" button, the task was failed with an error. After the fix the task is properly canceled and the job the task belongs to is canceled accordingly.
Summary by CodeRabbit
Release Notes