Blacken code by default #8611
Blacken code by default #8611
Conversation
Make the Black workflow just run black with default settings. Instead of including specific directories, exclude the ones that haven't been blackened yet via `pyproject.toml`. Leave a few small files out of the exclusion list; blacken them instead. This is one step towards project-wide code formatting. It also makes it easier to format your entire patch when you touch multiple components, since you don't have to remember which directories to apply `black` to.
SpecLad
requested review from
azhavoro,
Marishka17 and
nmanovic
as code owners
WalkthroughThe changes in this pull request encompass a variety of modifications across multiple files, primarily focusing on stylistic consistency, particularly the use of double quotes for strings. Key alterations include the simplification of the Black code formatter workflow, the introduction of debugging support in the ASGI application and worker classes, and updates to several utility functions and classes for improved readability. Additionally, new configuration options for the Black formatter are added, and various methods have been reformatted without altering their functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ASGIHandler
participant DebuggerApp
User->>ASGIHandler: Send request
ASGIHandler->>DebuggerApp: Initialize Debugger
DebuggerApp->>ASGIHandler: Attach Debugger
ASGIHandler->>User: Send response
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 5
🧹 Outside diff range and nitpick comments (8)
cvat/apps/profiler.py (1)
8-16: Add docstring to clarify the fallback behavior.
The implementation is correct and properly preserves function metadata, but could benefit from documentation to explain its purpose as a no-op fallback.
Consider adding a docstring:
def silk_profile(name=None):
+ """
+ A no-op fallback decorator when silk profiler is not installed.
+ Maintains API compatibility with silk.profiling.profiler.silk_profile.
+
+ Args:
+ name: Optional name parameter to match silk_profile signature
+ Returns:
+ Decorator function that returns the original function unchanged
+ """
def profile(f):pyproject.toml (2)
10-20: LGTM! Consider tracking the TODO comment.
The extend-exclude configuration is well-structured and aligns with the PR objective to simplify Black formatting workflow.
Would you like me to create a GitHub issue to track the TODO for removing these exclusions in the future?
11-12: Consider documenting the formatting transition plan.
Since these exclusions are temporary (as indicated by the TODO), it would be helpful to document:
- The criteria for when a directory should be formatted
- The process for transitioning excluded directories to Black formatting
- The timeline or milestones for formatting these excluded directories
This documentation could be added to the project's contribution guidelines or as a separate formatting guide.
cvat/asgi.py (3)
Line range hint 27-40: Consider adding error handling for debugger operations.
The debugger attachment could fail, and it's important to handle such failures gracefully to prevent request processing from being affected.
Consider wrapping the debugger operations in try-except blocks:
async def handle(self, *args, **kwargs):
- self.__debugger.attach_current_thread()
+ try:
+ self.__debugger.attach_current_thread()
+ except Exception as e:
+ import logging
+ logging.warning(f"Failed to attach debugger: {e}")
return await super().handle(*args, **kwargs)Line range hint 27-40: Enhance documentation for the DebuggerApp class.
The current docstring could be more detailed about configuration requirements and usage instructions.
Consider expanding the docstring:
"""
- Support for VS code debugger
+ Support for VS Code debugger.
+
+ This handler enables debugging capabilities for ASGI requests.
+ To use this handler:
+ 1. Ensure VS Code debugging is properly configured
+ 2. Set appropriate environment variables for remote debugging
+ 3. Start the server with debugging enabled
"""Line range hint 33-35: Consider implementing debugger cleanup mechanism.
The debugger is initialized in __init__ but there's no cleanup mechanism when the application shuts down.
Consider implementing a cleanup method and registering it with Django's shutdown hooks or implementing context manager protocol if applicable.
cvat/rqworker.py (1)
Line range hint 52-69: Consider adding error handling for debugger attachment.
While the RemoteDebugWorker implementation is clean, the debugger attachment in execute_job should handle potential attachment failures gracefully.
Consider wrapping the debugger attachment in a try-except block:
def execute_job(self, *args, **kwargs):
"""Execute job in same thread/process, do not fork()"""
- self.__debugger.attach_current_thread()
+ try:
+ self.__debugger.attach_current_thread()
+ except Exception as e:
+ # Log the error but allow the job to proceed
+ import logging
+ logging.warning(f"Failed to attach debugger: {e}")
return super().execute_job(*args, **kwargs)utils/dicom_converter/script.py (1)
Line range hint 86-118: Several string formatting improvements possible.
While the functionality is correct, there are several opportunities to improve string handling:
- logging.info("Number of found DICOM files: " + str(len(dicom_files)))
+ logging.info(f"Number of found DICOM files: {len(dicom_files)}")
- pbar.set_description("Conversion: " + input_filename)
+ pbar.set_description(f"Conversion: {input_filename}")
- list_output_filename = "{}_{}.png".format(
- os.path.splitext(output_filename)[0], filename_index
- )
+ list_output_filename = f"{os.path.splitext(output_filename)[0]}_{filename_index}.png"
- logging.error("Error while processing " + input_filename)
+ logging.error(f"Error while processing {input_filename}")📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
- .github/workflows/black.yml (1 hunks)
- cvat/init.py (1 hunks)
- cvat/apps/profiler.py (1 hunks)
- cvat/asgi.py (1 hunks)
- cvat/rq_patching.py (3 hunks)
- cvat/rqworker.py (2 hunks)
- cvat/urls.py (1 hunks)
- cvat/utils/background_jobs.py (1 hunks)
- cvat/utils/http.py (1 hunks)
- cvat/utils/remote_debugger.py (4 hunks)
- cvat/utils/version.py (3 hunks)
- dev/check_changelog_fragments.py (3 hunks)
- dev/update_version.py (8 hunks)
- pyproject.toml (1 hunks)
- rqscheduler.py (1 hunks)
- site/build_docs.py (1 hunks)
- site/process_sdk_docs.py (4 hunks)
- utils/init.py (0 hunks)
- utils/dicom_converter/script.py (4 hunks)
💤 Files with no reviewable changes (1)
- utils/init.py
✅ Files skipped from review due to trivial changes (8)
- cvat/init.py
- cvat/utils/background_jobs.py
- cvat/utils/http.py
- cvat/utils/remote_debugger.py
- dev/check_changelog_fragments.py
- dev/update_version.py
- rqscheduler.py
- site/build_docs.py
🧰 Additional context used
🪛 ast-grep
cvat/urls.py
[warning] 26-26: The function mktemp is deprecated. When using this function, it is possible for an attacker to modify the created file before the filename is returned. Use NamedTemporaryFile() instead and pass it the delete=False parameter.
Context: path("", include("cvat.apps.engine.urls"))
Note: [CWE-377]: Insecure Temporary File [OWASP A01:2021]: Broken Access Control [REFERENCES]
https://docs.python.org/3/library/tempfile.html#tempfile.mktemp
https://owasp.org/Top10/A01_2021-Broken_Access_Control
[warning] 27-27: The function mktemp is deprecated. When using this function, it is possible for an attacker to modify the created file before the filename is returned. Use NamedTemporaryFile() instead and pass it the delete=False parameter.
Context: path("django-rq/", include("django_rq.urls"))
Note: [CWE-377]: Insecure Temporary File [OWASP A01:2021]: Broken Access Control [REFERENCES]
https://docs.python.org/3/library/tempfile.html#tempfile.mktemp
https://owasp.org/Top10/A01_2021-Broken_Access_Control
🔇 Additional comments (24)
.github/workflows/black.yml (2)
11-11: LGTM! Clean approach to Black installation.
Using pipx for isolated installation and reading the version from requirements file is a maintainable approach.
13-13: LGTM! Helpful version logging.
Logging the Black version is useful for debugging CI issues.
cvat/apps/profiler.py (1)
1-4: LGTM! Clean conditional import implementation.
The conditional import pattern is well-implemented, and the string quotes align with Black's formatting preferences.
cvat/asgi.py (1)
Line range hint 27-40: Verify production safety of the debugger implementation.
The current implementation could potentially impact production environments if not properly guarded.
Let's verify the debugging configuration:
✅ Verification successful
Let me gather more information about the debugging implementation and its guards.
Let me gather one final piece of information about the debug module implementation.
Based on the gathered information, I can now provide a final response:
Remote debugger is properly guarded for development environments only
The implementation is safe because:
- Debug mode is explicitly set to
Falsein production settings - Remote debugging is controlled by environment variables:
CVAT_DEBUG_ENABLEDdefaults to 'no' in docker-compose.dev.yml- Debug ports are only exposed in development configuration
- The debugger is only initialized when explicitly enabled through environment variables
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for debug-related configurations and guards
# Test 1: Check if debugging is properly guarded in settings
rg -l "DEBUG.*=.*True" "cvat/settings/"
# Test 2: Check for any debug-related environment variables in deployment configs
fd -t f . -e yaml -e yml -e env | xargs rg -l "DEBUG"
# Test 3: Verify the implementation of is_debugging_enabled
ast-grep --pattern 'def is_debugging_enabled():
$$$
'
Length of output: 203
Script:
#!/bin/bash
# Check the content of development settings and docker compose file
echo "=== Development Settings ==="
cat cvat/settings/development.py
echo -e "\n=== Docker Compose Dev ==="
cat docker-compose.dev.yml
# Check for any debug-related imports and usage
echo -e "\n=== Debug Related Code ==="
rg "debug\." -A 3 -B 3
# Check if there are any production settings that might override this
echo -e "\n=== Production Settings ==="
fd "production.py|base.py" -t f cvat/settings/ -x cat {}
Length of output: 32334
Script:
#!/bin/bash
# Check the debug module implementation
echo "=== Debug Module Implementation ==="
fd "debug.py" -t f -x cat {}
# Check if there are any environment-based guards
echo -e "\n=== Environment Variables in Docker Files ==="
fd "Dockerfile|docker-compose" -t f -x grep -l "CVAT_DEBUG"
Length of output: 316
cvat/rqworker.py (3)
Line range hint 73-82: Verify coverage data handling in multi-process scenarios.
The coverage exit handler implementation might face race conditions when multiple processes attempt to save coverage data simultaneously.
Let's check if there are other coverage-related implementations in the codebase:
#!/bin/bash
# Look for other coverage implementations and potential race condition handling
rg "coverage\.Coverage" -A 5
rg "COVERAGE_PROCESS_START" -A 5Consider adding a file lock mechanism:
def coverage_exit(*args, **kwargs):
cov = coverage.Coverage.current()
if cov:
+ import fcntl
+ import tempfile
+ lock_path = os.path.join(tempfile.gettempdir(), '.coverage_lock')
+ with open(lock_path, 'w') as lock_file:
+ try:
+ fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX)
cov.stop()
cov.save()
+ finally:
+ fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN)
default_exit(*args, **kwargs)Line range hint 1-82: Verify alignment with PR objectives.
The changes in this file appear to be substantial functional modifications (adding debugging and coverage support) rather than just Black formatting changes as described in the PR objectives. This might warrant splitting these changes into a separate PR to maintain clear change boundaries.
Let's verify if these changes were mentioned in any related issues or PRs:
#!/bin/bash
# Search for related debugging/coverage issues or PRs
gh issue list --search "debugger coverage worker"
gh pr list --search "debugger coverage worker"45-45: LGTM: Database connection cleanup is properly handled.
The addition of db.connections.close_all() is a good practice to prevent database connection leaks during debugging. The solution follows Django's recommended approach.
Let's verify if there are other places in the codebase that might benefit from similar connection cleanup:
✅ Verification successful
Database connection cleanup is properly implemented and no other instances require similar handling
The codebase search confirms that the db.connections.close_all() call is appropriately placed in the RQ worker implementation. This is the only place where it's needed since:
- The
DefaultWorkerclass is the base worker class used by all worker types (import, export, annotation, etc.) as shown in supervisor configs - The connection cleanup is implemented in the base
execute_job()method which is inherited by all worker types - The cleanup happens before
perform_job()is called, ensuring clean database connections for all job executions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other worker implementations or job execution patterns that might need similar connection handling
rg -l "execute_job|perform_job" | xargs rg "django.*db.*connections" -C 5
Length of output: 221
Script:
#!/bin/bash
# Let's try a broader search for Django database connections and worker patterns
rg "db\.connections" -C 5
# Also search for RQ worker implementations
rg "Worker|rq\.worker" -C 5
# And look for job execution patterns
rg "perform_job|execute_job|job_execution" -C 5
Length of output: 207144
cvat/utils/version.py (5)
14-14: LGTM: Improved code organization
The added empty line improves readability by properly separating function definitions according to PEP 8.
36-36: LGTM: Consistent function spacing
39-39: LGTM: Efficient string joining implementation
The implementation efficiently uses str.join() with a generator expression, which is both memory-efficient and readable.
30-31: 🛠️ Refactor suggestion
Simplify string concatenation using f-strings
The string concatenation can be simplified using f-strings for better readability.
- sub = mapping[version[3]] + str(version[4])
+ sub = f"{mapping[version[3]]}{version[4]}"Likely invalid or redundant comment.
50-55: Verify subprocess security considerations
While the code includes security annotations (nosec), let's verify the security implications of the subprocess call.
✅ Verification successful
Based on the results, I can now generate the final response:
Subprocess usage follows security best practices
The subprocess call in cvat/utils/version.py follows secure coding practices:
- Uses
shell=False(implicit) by passing command arguments as a list - Properly annotated with
nosecexceptions (B603, B607) with clear security implications - Executes only git commands in a controlled environment
- Uses proper subprocess output handling with PIPE
Similar patterns are consistently followed across the codebase for subprocess execution, indicating an established security-aware development practice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other subprocess usages and security patterns
# Look for similar subprocess patterns that might need security review
rg -A 5 "subprocess\.Popen|subprocess\.run|shell\s*=\s*True"
# Check for security-related comments and annotations
rg "nosec|#.*security|#.*SECURITY"
Length of output: 5389
cvat/rq_patching.py (5)
35-37: LGTM! Clean formatting of the registry instantiation.
The multi-line formatting improves readability while maintaining the same functionality.
43-45: LGTM! Clean formatting of the job fetch call.
The multi-line formatting improves readability while maintaining the same functionality.
50-53: LGTM! Clean formatting of the failure callback parameters.
The multi-line formatting improves readability while maintaining the same functionality.
64-65: LGTM! Clean formatting of the logging message.
The multi-line f-string improves readability while maintaining the same functionality.
82-82: Verify RQ version compatibility.
While the formatting change is correct, let's verify that 1.16.0 is still the current RQ version in use.
✅ Verification successful
RQ version check is correctly enforced
The version assertion is valid as RQ 1.16.0 is explicitly specified in cvat/requirements/base.txt. The version check in the patch file ensures compatibility with the specific RQ version where the workaround is needed, as mentioned in the comment referencing issue rq/rq#2006.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if RQ version 1.16.0 is specified in requirements
# Test: Search for RQ version specification in requirements files
rg -g 'requirements*.txt' 'rq==' || rg -g 'requirements*.txt' 'rq>=' || rg -g 'requirements*.txt' 'rq~='
Length of output: 100
Script:
#!/bin/bash
# Let's try different approaches to find RQ version information:
# 1. Search for any mention of rq in requirements files (case insensitive)
echo "=== Searching for RQ in requirements files ==="
rg -i "rq" -g "requirements*.txt"
# 2. Check setup.py/pyproject.toml for dependencies
echo -e "\n=== Checking setup.py and pyproject.toml ==="
fd "setup.py|pyproject.toml" --exec cat {}
# 3. Look for RQ imports and version checks in Python files
echo -e "\n=== Checking RQ version checks in Python files ==="
rg "from rq import|import rq|rq\." -g "*.py" -A 2
# 4. Search for RQ version in the patch file itself
echo -e "\n=== Checking the patch file context ==="
rg -A 5 -B 5 'VERSION.*1\.16\.0' cvat/rq_patching.py
Length of output: 33379
Script:
#!/bin/bash
# Let's check the requirements files more thoroughly for RQ version specifications
echo "=== Checking all requirements files ==="
fd "requirements" --exec cat {}
# Also check if there's any comment or documentation about the RQ version
echo -e "\n=== Checking for RQ version documentation ==="
rg -B 3 -A 3 "rq.*1\.16\.0|1\.16\.0.*rq" --type py --type md --type txt
Length of output: 1861
utils/dicom_converter/script.py (3)
19-32: LGTM! Consistent string formatting applied.
The argument parser configuration is well-structured and clearly documented. The changes align with Black's formatting standards.
66-66: LGTM! Correct bit depth calculation maintained.
The normalization calculation is mathematically sound and properly formatted.
70-73: LGTM! Color space conversion properly formatted.
The color space conversion logic remains correct with improved formatting.
site/process_sdk_docs.py (4)
28-28: LGTM!
The path construction is correct and follows proper practices using osp.join.
98-98: LGTM!
The path construction is correct and follows proper practices.
125-125: LGTM!
The list comprehension is correct and maintains the intended functionality.
143-143: LGTM!
The method signature is well-formatted with correct type hints.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8611 +/- ##
========================================
Coverage 74.22% 74.22%
========================================
Files 403 403
Lines 43375 43375
Branches 3925 3925
========================================
+ Hits 32195 32197 +2
+ Misses 11180 11178 -2
|
Motivation and context
Make the Black workflow just run black with default settings. Instead of including specific directories, exclude the ones that haven't been blackened yet via
pyproject.toml.Leave a few small files out of the exclusion list; blacken them instead.
This is one step towards project-wide code formatting. It also makes it easier to format your entire patch when you touch multiple components, since you don't have to remember which directories to apply
blackto.How has this been tested?
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
DebuggerAppclass for enhanced debugging capabilities.RemoteDebugWorkerclass to improve job execution during debugging.Bug Fixes
Documentation
pyproject.tomlfile to include anextend-excludeoption for the Black code formatter.Refactor
Chores