Fixed: Queued jobs are not considered in deferring logic #7907
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes enhance the robustness of job handling and formatting configurations in the CVAT codebase. Key updates include refined job cancellation logic, improved exception handling, and optimized database connection management. Additionally, specific settings for Python code formatting were removed from the project configuration. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (3)
cvat/apps/engine/views.py (3)
Line range hint
44-44: Remove unused import to clean up the code.- import cvat.apps.dataset_manager.views # pylint: disable=unused-import
Line range hint
1967-1967: This f-string does not contain placeholders, which might be an oversight. Please verify if dynamic data was intended to be included.
Line range hint
2714-2714: This f-string does not contain placeholders, which might be an oversight. Please verify if dynamic data was intended to be included.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .vscode/settings.json (1 hunks)
- cvat/apps/engine/views.py (1 hunks)
- cvat/apps/events/handlers.py (1 hunks)
- cvat/rqworker.py (3 hunks)
Files skipped from review due to trivial changes (1)
- .vscode/settings.json
Additional Context Used
Ruff (4)
cvat/apps/engine/views.py (4)
44-44:
cvat.apps.dataset_manager.viewsimported but unused
1967-1967: f-string without any placeholders
2714-2714: f-string without any placeholders
2976-2976: Undefined name
send_stop_job_command
Additional comments not posted (5)
cvat/rqworker.py (4)
7-7: Import ofsignalmodule is appropriate for handling termination signals.
46-46: Closing all database connections before job execution is a good practice to prevent operational errors.
49-52: Changing the termination signal to SIGTERM is a thoughtful approach for graceful debugging. Ensure this change aligns with other parts of the application.Verification successful
The handling of SIGTERM in
cvat/rqworker.pyandcvat/apps/events/handlers.pyis consistent. Thekill_horsemethod sends SIGTERM for graceful termination, andhandle_rq_exceptionavoids logging intentional SIGTERM exits.
cvat/rqworker.py: Uses SIGTERM for graceful termination in debug mode.cvat/apps/events/handlers.py: Avoids logging intentional SIGTERM exits.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that SIGTERM handling is consistent across the application. # Test: Search for SIGTERM handling in Python files. Expect: Consistent handling logic. rg --type python 'SIGTERM'Length of output: 57
Script:
#!/bin/bash # Description: Verify that SIGTERM handling is consistent across the application. # Test: Search for SIGTERM handling in Python files. Expect: Consistent handling logic. rg --type py 'SIGTERM'Length of output: 398
Script:
#!/bin/bash # Description: Extract relevant code for SIGTERM handling in cvat/apps/events/handlers.py # Test: Display lines around the SIGTERM handling comment to understand the context. rg --context 5 'SIGTERM' cvat/apps/events/handlers.pyLength of output: 462
56-56: Introduction ofRemoteDebugWorkerclass enhances support for remote debugging with VS Code.cvat/apps/events/handlers.py (1)
524-527: HandlingSystemExitto avoid logging intentional job terminations is a prudent enhancement for cleaner logs.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #7907 +/- ##
===========================================
+ Coverage 83.47% 83.51% +0.04%
===========================================
Files 384 384
Lines 40320 40325 +5
Branches 3772 3772
===========================================
+ Hits 33656 33679 +23
+ Misses 6664 6646 -18
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- cvat/apps/engine/views.py (2 hunks)
- cvat/rqworker.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat/rqworker.py
Additional Context Used
Ruff (3)
cvat/apps/engine/views.py (3)
45-45:
cvat.apps.dataset_manager.viewsimported but unused
1968-1968: f-string without any placeholders
2715-2715: f-string without any placeholders
Additional comments not posted (2)
cvat/apps/engine/views.py (2)
19-19: Ensure the new importsend_stop_job_commandis utilized correctly.Verification successful
Verification of
send_stop_job_commandUsageThe
send_stop_job_commandfunction is correctly imported and utilized in the codebase.
- Import Statement:
cvat/apps/engine/views.py:from rq.command import send_stop_job_command- Usage:
cvat/apps/engine/views.py: send_stop_job_command(rq_job.connection, rq_job.id)The function is used appropriately in the file where it is imported.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `send_stop_job_command` function. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python $'send_stop_job_command'Length of output: 71
Script:
#!/bin/bash # Description: Verify the usage of `send_stop_job_command` function. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py 'send_stop_job_command'Length of output: 201
2969-2984: The logic to handle job cancellation or stopping seems to be correctly implemented. However, ensure that thesend_stop_job_commandfunction is defined and imported correctly as it is newly introduced.
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/rqworker.py (2 hunks)
Additional comments not posted (3)
cvat/rqworker.py (3)
7-7: Added import forsignalmodule.This is necessary for the new signal handling logic introduced in this file.
50-54: Introducedkill_horsemethod to sendSIGTERMinstead ofSIGKILL.This change allows for a more graceful termination of processes, especially useful in debug mode where
SIGKILLcannot be intercepted. This aligns with the PR's goal to manage process termination more effectively.
68-73: Enhancedhandle_exceptionto ignore logging for intentionally stopped jobs.This change prevents unnecessary logging for jobs that are intentionally stopped, which can help in reducing noise in the logs and focusing on actual errors.
cvat/rqworker.py
Outdated
| def handle_job_failure(self, *args, **kwargs): | ||
| # export job with the same ID was re-queued in the main process | ||
| # we do not need to handle failure | ||
| is_stopped_export_job = kwargs['queue'].name == 'export' and kwargs['exc_string'].strip().split('\n')[-1] == 'rq.worker.StopRequested' | ||
| signal.signal(signal.SIGTERM, self.request_stop) | ||
| if not is_stopped_export_job: | ||
| super().handle_job_failure(*args, **kwargs) | ||
|
|
||
| # after the first warm stop (StopRequested), default code reassignes SIGTERM signal to cold stop (SysExit) | ||
| # we still want use warm stops in debug process | ||
| signal.signal(signal.SIGTERM, self.request_stop) | ||
|
|
There was a problem hiding this comment.
Modified handle_job_failure to handle specific cases for export jobs.
The logic to check if the job is an export job and whether it was stopped due to a StopRequested exception is crucial. However, the implementation could be simplified and made more robust by encapsulating this logic into a separate method. This would improve readability and maintainability.
- is_stopped_export_job = kwargs['queue'].name == 'export' and kwargs['exc_string'].strip().split('\n')[-1] == 'rq.worker.StopRequested'
+ is_stopped_export_job = self.is_stopped_export_job(kwargs)
def is_stopped_export_job(self, kwargs):
return kwargs['queue'].name == 'export' and kwargs['exc_string'].strip().split('\n')[-1] == 'rq.worker.StopRequested'Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def handle_job_failure(self, *args, **kwargs): | |
| # export job with the same ID was re-queued in the main process | |
| # we do not need to handle failure | |
| is_stopped_export_job = kwargs['queue'].name == 'export' and kwargs['exc_string'].strip().split('\n')[-1] == 'rq.worker.StopRequested' | |
| signal.signal(signal.SIGTERM, self.request_stop) | |
| if not is_stopped_export_job: | |
| super().handle_job_failure(*args, **kwargs) | |
| # after the first warm stop (StopRequested), default code reassignes SIGTERM signal to cold stop (SysExit) | |
| # we still want use warm stops in debug process | |
| signal.signal(signal.SIGTERM, self.request_stop) | |
| def handle_job_failure(self, *args, **kwargs): | |
| # export job with the same ID was re-queued in the main process | |
| # we do not need to handle failure | |
| is_stopped_export_job = self.is_stopped_export_job(kwargs) | |
| signal.signal(signal.SIGTERM, self.request_stop) | |
| if not is_stopped_export_job: | |
| super().handle_job_failure(*args, **kwargs) | |
| # after the first warm stop (StopRequested), default code reassignes SIGTERM signal to cold stop (SysExit) | |
| # we still want use warm stops in debug process | |
| signal.signal(signal.SIGTERM, self.request_stop) | |
| def is_stopped_export_job(self, kwargs): | |
| return kwargs['queue'].name == 'export' and kwargs['exc_string'].strip().split('\n')[-1] == 'rq.worker.StopRequested' |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/rqworker.py (3 hunks)
Additional comments not posted (5)
cvat/rqworker.py (5)
13-18: The override of the default signal handling to always use cold shutdown is clear and aligns with the PR's objectives.
19-26: The handling of job failures with consideration for intentionally stopped jobs is well-implemented.
66-69: Modifying the signal to SIGTERM in debug mode is a thoughtful change that facilitates better debugging.
71-80: The exception handling logic to differentiate between intentional process termination and actual exceptions is well-crafted.
81-81: Enabling a special worker for remote debugging conditionally is a prudent approach that isolates debugging features from production.
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
cvat/rqworker.py (1)
77-77: Ensure that debugging and coverage features are well-documented.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/rqworker.py (3 hunks)
Additional comments not posted (2)
cvat/rqworker.py (2)
62-65: LGTM! Good use of SIGTERM for graceful termination in debug mode.
40-40: Ensure database connections are properly managed to prevent operational errors.
cvat/rqworker.py
Outdated
| def handle_job_failure(self, job, queue, **kwargs): | ||
| # pylint: disable=access-member-before-definition | ||
| if self._stopped_job_id == job.id: | ||
| self._stopped_job_id = None | ||
| self.set_current_job_id(None) | ||
| else: | ||
| # the job was stopped intentionally, we do not need update its status or put it into failed registry | ||
| # in our case the job is immediately removed after stop request | ||
| super().handle_job_failure(job, queue, **kwargs) |
There was a problem hiding this comment.
Ensure clarity by adding a comment explaining the check for _stopped_job_id.
+ # Check if the job was stopped intentionally and handle accordinglyCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def handle_job_failure(self, job, queue, **kwargs): | |
| # pylint: disable=access-member-before-definition | |
| if self._stopped_job_id == job.id: | |
| self._stopped_job_id = None | |
| self.set_current_job_id(None) | |
| else: | |
| # the job was stopped intentionally, we do not need update its status or put it into failed registry | |
| # in our case the job is immediately removed after stop request | |
| super().handle_job_failure(job, queue, **kwargs) | |
| def handle_job_failure(self, job, queue, **kwargs): | |
| # pylint: disable=access-member-before-definition | |
| # Check if the job was stopped intentionally and handle accordingly | |
| if self._stopped_job_id == job.id: | |
| self._stopped_job_id = None | |
| self.set_current_job_id(None) | |
| else: | |
| # the job was stopped intentionally, we do not need update its status or put it into failed registry | |
| # in our case the job is immediately removed after stop request | |
| super().handle_job_failure(job, queue, **kwargs) |
cvat/rqworker.py
Outdated
| def handle_exception(self, *args, **kwargs): | ||
| # on production it sends SIGKILL to work horse process | ||
| # but for development we overrided it and it sends SIGTERM to the process | ||
| # we need to prevent exception handling as the process killed intentilnally | ||
|
|
||
| # moreover default code saves meta with exception | ||
| # and rewrites request datetime in meta with old value, as new job with the same ID may aldeady been created in a new process | ||
| is_stopped_export_job = isinstance(args[2], (StopRequested, SystemExit)) | ||
| if not is_stopped_export_job: | ||
| super().handle_exception(*args, **kwargs) |
There was a problem hiding this comment.
Clarify the exception handling logic with a detailed comment.
+ # Check if the exception is due to a stop request or system exit and handle accordinglyCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def handle_exception(self, *args, **kwargs): | |
| # on production it sends SIGKILL to work horse process | |
| # but for development we overrided it and it sends SIGTERM to the process | |
| # we need to prevent exception handling as the process killed intentilnally | |
| # moreover default code saves meta with exception | |
| # and rewrites request datetime in meta with old value, as new job with the same ID may aldeady been created in a new process | |
| is_stopped_export_job = isinstance(args[2], (StopRequested, SystemExit)) | |
| if not is_stopped_export_job: | |
| super().handle_exception(*args, **kwargs) | |
| def handle_exception(self, *args, **kwargs): | |
| # on production it sends SIGKILL to work horse process | |
| # but for development we overrided it and it sends SIGTERM to the process | |
| # we need to prevent exception handling as the process killed intentilnally | |
| # moreover default code saves meta with exception | |
| # and rewrites request datetime in meta with old value, as new job with the same ID may aldeady been created in a new process | |
| # Check if the exception is due to a stop request or system exit and handle accordingly | |
| is_stopped_export_job = isinstance(args[2], (StopRequested, SystemExit)) | |
| if not is_stopped_export_job: | |
| super().handle_exception(*args, **kwargs) |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/rqworker.py (3 hunks)
Additional comments not posted (3)
cvat/rqworker.py (3)
14-22: The changes inhandle_job_failurealign well with the PR's objective to manage resources more efficiently by handling stopped jobs appropriately.
62-66: The modification to send SIGTERM instead of SIGKILL in debug mode is a thoughtful addition for better debugging experience.
81-81: The conditional structures for debugging and coverage are well-implemented and necessary for development and testing environments.
bsekachev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/engine/backup.py (2 hunks)
Additional Context Used
Ruff (3)
cvat/apps/engine/backup.py (3)
92-92: f-string without any placeholders
392-392: Ambiguous variable name:
l
797-797: Ambiguous variable name:
l
Additional comments not posted (1)
cvat/apps/engine/backup.py (1)
968-973: Ensure proper handling of job cancellation and dependent job enqueuing.The logic to handle job cancellation based on its status is correctly implemented. It checks if the job is started and sends a stop command, otherwise, it cancels the job and enqueues dependent jobs if configured. This aligns well with the PR's objective to manage resources more efficiently by terminating unnecessary processes.
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/rq_patching.py (2 hunks)
Additional Context Used
Ruff (1)
cvat/rq_patching.py (1)
17-17:
cvat.utils.remote_debuggerimported but unused
Additional comments not posted (1)
cvat/rq_patching.py (1)
77-77: Consider using a single timestamp for all jobs in the cleanup process.- job._exc_info = f"Moved to {rq.registry.FailedJobRegistry.__name__}, {exc_string}, at {datetime.now()}" + current_time = datetime.now() + job._exc_info = f"Moved to {rq.registry.FailedJobRegistry.__name__}, {exc_string}, at {current_time}"This change ensures that all jobs processed in a single cleanup cycle share the same timestamp, which can be beneficial for debugging and consistency.
Likely invalid or redundant comment.
.vscode/settings.json
Outdated
| "isort.args":["--profile", "black"], | ||
| "[python]": { | ||
| "editor.defaultFormatter": "ms-python.black-formatter", | ||
| "editor.formatOnSave": true | ||
| } |
There was a problem hiding this comment.
I added these settings previously, but now it looks like they bring more problems than profit
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (5)
cvat/apps/engine/backup.py (2)
Line range hint
94-94: Remove unnecessary f-string.- logger.info(f"The {obj_type} '{db_instance.name}' is backuped at '{output_path}' " + + logger.info("The {} '{}' is backuped at '{}' ".format(obj_type, db_instance.name, output_path) +
Line range hint
394-394: Clarify ambiguous variable name 'l'.- for l in labels: + for label in labels:cvat/apps/engine/views.py (3)
Line range hint
47-47: Remove unused import to clean up the code.- import cvat.apps.dataset_manager.views # pylint: disable=unused-import
Line range hint
1970-1970: Remove unnecessary f-string notation.- raise serializers.ValidationError(f"Unexpected location {location} specified for the request") + raise serializers.ValidationError("Unexpected location specified for the request")
Line range hint
2717-2717: Remove unnecessary f-string notation.- raise NotImplementedError(f"Export to {location} location is not implemented yet") + raise NotImplementedError("Export to specified location is not implemented yet")
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- cvat/apps/engine/backup.py (3 hunks)
- cvat/apps/engine/views.py (4 hunks)
Additional Context Used
Ruff (6)
cvat/apps/engine/backup.py (3)
94-94: f-string without any placeholders
394-394: Ambiguous variable name:
l
799-799: Ambiguous variable name:
lcvat/apps/engine/views.py (3)
47-47:
cvat.apps.dataset_manager.viewsimported but unused
1970-1970: f-string without any placeholders
2717-2717: f-string without any placeholders
Additional comments not posted (2)
cvat/apps/engine/backup.py (2)
2-2: Update the copyright year to reflect the current year.- # Copyright (C) 2022-2024 CVAT.ai Corporation + # Copyright (C) 2022-2025 CVAT.ai CorporationLikely invalid or redundant comment.
973-974: Use contextlib.suppress to handle exceptions more gracefully.- with suppress(InvalidJobOperation): - send_stop_job_command(rq_job.connection, rq_job.id) + with suppress(InvalidJobOperation): + send_stop_job_command(rq_job.connection, rq_job.id)Likely invalid or redundant comment.
cvat/apps/engine/views.py
Outdated
| with suppress(InvalidJobOperation): | ||
| send_stop_job_command(rq_job.connection, rq_job.id) |
There was a problem hiding this comment.
Ensure proper handling of send_stop_job_command function.
The function send_stop_job_command is used here, but there is no import statement for it in the provided code. Ensure that this function is defined or imported correctly to avoid runtime errors.
There was a problem hiding this comment.
Really? Check line number 20
There was a problem hiding this comment.
@bsekachev, you are correct. The send_stop_job_command function is indeed imported at line 20 in the file cvat/apps/engine/views.py. My initial comment was incorrect. Thank you for pointing that out.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition or import of 'send_stop_job_command' in the file 'cvat/apps/engine/views.py'
rg "send_stop_job_command" cvat/apps/engine/views.py
Length of output: 165
New Learnings Added
User: bsekachev
PR: cvat-ai/cvat#7907
File: cvat/apps/engine/views.py:2982-2983
Timestamp: 2024-05-22T13:55:59.926Z
Learning: The `send_stop_job_command` function is imported from `rq.command` at line 20 in the file `cvat/apps/engine/views.py`.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/engine/utils.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat/apps/engine/utils.py
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/engine/utils.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat/apps/engine/utils.py
Co-authored-by: Maria Khrustaleva <maria@cvat.ai>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/engine/utils.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat/apps/engine/utils.py
cvat/rqworker.py
Outdated
| # This modification ensures that jobs stopped intentionally | ||
| # do not get their status updated or placed in the failed registry | ||
| # as the main server code must delete them at all | ||
| def handle_job_failure(self, job, queue, **kwargs): | ||
| # pylint: disable=access-member-before-definition | ||
| if self._stopped_job_id == job.id: | ||
| self._stopped_job_id = None | ||
| self.set_current_job_id(None) | ||
| else: | ||
| super().handle_job_failure(job, queue, **kwargs) |
There was a problem hiding this comment.
I see one problem here - a started rq job can also be stopped from django admin panel and with these changes such a job won't be handled correctly.
bsekachev
changed the title
|
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Bug Fixes
handle_rq_exceptionfunction to handleSystemExitexceptions.New Features
SystemExit.Performance Improvements
Chores