[CI] Moved and Updated Python Lint Test #3380
Conversation
Using this as a stepping stone before updating to a higher version.
github-actions
bot
added
VPR
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR modernizes the codebase through CI workflow restructuring, pylint version upgrade from 2.7.4 to 4.0.4, systematic UTF-8 encoding standardization across file I/O operations, extensive whitespace cleanup, and minor code quality improvements including better exception handling and context manager usage. Changes
Possibly Related PRs
Suggested Reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (4)
vtr_flow/benchmarks/system_verilog/f4pga/make_sv_flattened.py (1)
18-44: Consider adding error handling for file operations.While the UTF-8 encoding additions are excellent, the file operations lack error handling for scenarios such as missing files, permission issues, or I/O errors. Adding try-except blocks around file operations would improve robustness.
🔎 Example error handling pattern
try: with open(file, "r", encoding="utf-8") as f: for line in f: if top_module_regex.search(line): return file except (IOError, OSError) as e: print(f"Warning: Could not read {file}: {e}") continuevtr_flow/scripts/noc/noc_benchmark_test.py (1)
299-299: LGTM! More specific exception types improve error clarity.Changing from generic exceptions to
ValueErroris appropriate for these parsing validation errors. The descriptive messages clearly indicate what went wrong.Static analysis suggests avoiding long messages outside exception classes (TRY003). While the current approach is acceptable, you could optionally define custom exception classes if these errors need structured handling in the future:
Optional refactor to custom exception classes
class PlacementDataError(ValueError): """Base class for placement data parsing errors""" pass class PlacementCostError(PlacementDataError): """Raised when placement cost is not written correctly""" pass # Then use: raise PlacementCostError("Placement cost not written out correctly")Also applies to: 323-323, 343-343, 367-367, 384-384, 401-401, 418-418
dev/pylint_check.py (1)
224-226: Consider documenting the rationale for 30 positional arguments.While the PR description mentions that many files use 25+ positional arguments, setting the limit to 30 is quite high compared to typical best practices (5-7 arguments). This may hide legitimate code complexity issues.
Consider adding a comment explaining why this high limit is necessary and whether there's a plan to refactor functions with excessive parameters in the future:
🔎 Suggested documentation
# Increase the max number of positional arguments. + # Many legacy functions in this codebase use 25+ positional arguments. + # TODO: Refactor these functions to use fewer parameters or configuration objects. cmd.append("--max-positional-arguments=30")vtr_flow/scripts/python_libs/vtr/parse_vtr_task.py (1)
452-453: Consider removing the pylint directive if.keys()is intentional.The
pylint: disable-next=consider-using-dict-itemsdirective suppresses a warning about iterating overpass_requirements.keys(). Since only the keys are used in the loop (line 453), using.keys()is actually appropriate here and shouldn't trigger a warning. However, if this warning does appear with pylint 4.0.4, the directive is acceptable.If the pylint warning is no longer triggered with the updated version, consider removing the directive:
🔎 Potential simplification
- first_fail = True - # pylint: disable-next=consider-using-dict-items - for metric in pass_requirements.keys(): + first_fail = True + for metric in pass_requirements.keys():Alternatively, if the directive is needed, consider adding a comment explaining why:
first_fail = True + # Only keys are needed here, not values # pylint: disable-next=consider-using-dict-items for metric in pass_requirements.keys():
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
.github/workflows/lint.yml(1 hunks).github/workflows/test.yml(0 hunks)dev/annealing_curve_plotter.py(0 hunks)dev/external_subtrees.py(0 hunks)dev/pylint_check.py(2 hunks)dev/submit_slurm.py(0 hunks)dev/vtr_test_suite_verifier/verify_test_suites.py(2 hunks)doc/src/vtr_version.py(0 hunks)odin_ii/regression_test/parse_result/conf/hooks.py(0 hunks)odin_ii/regression_test/parse_result/parse_result.py(0 hunks)odin_ii/regression_test/tools/odin_config_maker.py(0 hunks)odin_ii/regression_test/tools/parse_odin_result.py(0 hunks)requirements.txt(1 hunks)run_reg_test.py(0 hunks)vpr/scripts/compare_timing_reports.py(0 hunks)vpr/scripts/profile/parse_and_plot_detailed.py(0 hunks)vtr_flow/benchmarks/system_verilog/f4pga/make_sv_flattened.py(2 hunks)vtr_flow/scripts/arch_gen/arch_gen.py(0 hunks)vtr_flow/scripts/benchtracker/flask_cors/core.py(0 hunks)vtr_flow/scripts/benchtracker/interface_db.py(0 hunks)vtr_flow/scripts/benchtracker/populate_db.py(0 hunks)vtr_flow/scripts/blif_splicer.py(0 hunks)vtr_flow/scripts/download_ispd.py(0 hunks)vtr_flow/scripts/download_symbiflow.py(1 hunks)vtr_flow/scripts/download_titan.py(0 hunks)vtr_flow/scripts/ispd2vtr.py(0 hunks)vtr_flow/scripts/noc/noc_benchmark_test.py(10 hunks)vtr_flow/scripts/python_libs/vtr/log_parse.py(3 hunks)vtr_flow/scripts/python_libs/vtr/parse_vtr_flow.py(1 hunks)vtr_flow/scripts/python_libs/vtr/parse_vtr_task.py(4 hunks)vtr_flow/scripts/python_libs/vtr/task.py(1 hunks)vtr_flow/scripts/python_libs/vtr/util.py(9 hunks)vtr_flow/scripts/qor_compare.py(0 hunks)vtr_flow/scripts/run_vtr_flow.py(3 hunks)vtr_flow/scripts/run_vtr_task.py(3 hunks)vtr_flow/scripts/tuning_runs/control_runs.py(2 hunks)vtr_flow/scripts/upgrade_arch.py(0 hunks)
💤 Files with no reviewable changes (22)
- odin_ii/regression_test/tools/odin_config_maker.py
- dev/annealing_curve_plotter.py
- .github/workflows/test.yml
- vtr_flow/scripts/arch_gen/arch_gen.py
- vtr_flow/scripts/download_ispd.py
- vpr/scripts/compare_timing_reports.py
- odin_ii/regression_test/parse_result/conf/hooks.py
- vpr/scripts/profile/parse_and_plot_detailed.py
- odin_ii/regression_test/parse_result/parse_result.py
- dev/submit_slurm.py
- vtr_flow/scripts/benchtracker/populate_db.py
- vtr_flow/scripts/blif_splicer.py
- vtr_flow/scripts/download_titan.py
- dev/external_subtrees.py
- run_reg_test.py
- odin_ii/regression_test/tools/parse_odin_result.py
- vtr_flow/scripts/benchtracker/flask_cors/core.py
- vtr_flow/scripts/upgrade_arch.py
- vtr_flow/scripts/benchtracker/interface_db.py
- doc/src/vtr_version.py
- vtr_flow/scripts/ispd2vtr.py
- vtr_flow/scripts/qor_compare.py
🧰 Additional context used
🧬 Code graph analysis (1)
vtr_flow/scripts/python_libs/vtr/parse_vtr_task.py (2)
vtr_flow/scripts/python_libs/vtr/parse_vtr_flow.py (1)
parse_vtr_flow(45-97)vtr_flow/scripts/python_libs/vtr/task.py (4)
parse_command(166-170)second_parse_command(172-176)work_dir(184-188)qor_parse_command(178-182)
🪛 Ruff (0.14.8)
vtr_flow/scripts/download_symbiflow.py
115-115: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
vtr_flow/scripts/noc/noc_benchmark_test.py
299-299: Avoid specifying long messages outside the exception class
(TRY003)
323-323: Avoid specifying long messages outside the exception class
(TRY003)
343-343: Avoid specifying long messages outside the exception class
(TRY003)
367-367: Avoid specifying long messages outside the exception class
(TRY003)
384-384: Avoid specifying long messages outside the exception class
(TRY003)
401-401: Avoid specifying long messages outside the exception class
(TRY003)
418-418: Avoid specifying long messages outside the exception class
(TRY003)
535-535: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: B: Clang 18 (Ubuntu Noble - 24.04)
- GitHub Check: R: Strong
- GitHub Check: B: GCC 14 (Ubuntu Noble - 24.04)
- GitHub Check: R: SystemVerilog
- GitHub Check: B: GCC 12 (Ubuntu Noble - 24.04)
- GitHub Check: R: Valgrind Memory
- GitHub Check: Ubuntu Jammy - 22.04 Compatibility Test
- GitHub Check: Parmys Basic Test
- GitHub Check: R: Strong_odin
- GitHub Check: R: Valgrind Memory Odin
- GitHub Check: ODIN-II Basic Test
- GitHub Check: S: Basic
- GitHub Check: B: Build Variations
- GitHub Check: Image
🔇 Additional comments (24)
vtr_flow/benchmarks/system_verilog/f4pga/make_sv_flattened.py (2)
18-18: LGTM! UTF-8 encoding standardization.The explicit UTF-8 encoding improves cross-platform compatibility and follows best practices for file I/O operations.
30-44: LGTM! Consistent UTF-8 encoding across all file operations.The explicit UTF-8 encoding for both reading and writing operations ensures consistent text handling across different platforms and follows Python best practices.
vtr_flow/scripts/tuning_runs/control_runs.py (1)
64-66: LGTM! Encoding standardization improves text handling.The explicit UTF-8 encoding added to file operations ensures consistent text handling across different platforms and prevents potential encoding issues.
Also applies to: 244-244
dev/vtr_test_suite_verifier/verify_test_suites.py (1)
53-53: LGTM! Consistent encoding standardization.Explicit UTF-8 encoding added to file operations aligns with the repository-wide standardization effort and ensures reliable text decoding.
Also applies to: 81-81
vtr_flow/scripts/python_libs/vtr/parse_vtr_flow.py (1)
36-36: LGTM! Encoding added for consistent text handling.vtr_flow/scripts/run_vtr_flow.py (3)
509-509: LGTM! UTF-8 encoding added for reliable text decoding.
596-599: LGTM! KeyboardInterrupt handling simplified correctly.Removing the unused error variable binding is appropriate since the exception isn't used in the handler.
800-800: LGTM! Variable initialization ensures it exists on all paths.Initializing
return_status = Noneat the start of the function ensures the variable is defined on all code paths, preventing potential UnboundLocalError.vtr_flow/scripts/python_libs/vtr/log_parse.py (2)
10-10: LGTM! Simplified import is appropriate for modern Python.The direct import of
xml.etree.ElementTreeis correct. The C-basedcElementTreemodule was deprecated in Python 3.9+ and merged into ElementTree, so the conditional import is no longer necessary.
427-427: LGTM! UTF-8 encoding standardization.Explicit UTF-8 encoding added to file operations ensures consistent text handling.
Also applies to: 526-526
vtr_flow/scripts/python_libs/vtr/util.py (3)
184-192: LGTM! Pylint directive is justified.The
consider-using-withdirective is appropriately disabled here because the subprocess needs to be managed manually to read from stdout in a loop and handle timeouts. The comment about PWD environment variable is also helpful context.
266-266: LGTM! Comprehensive UTF-8 encoding standardization.All file operations now explicitly specify UTF-8 encoding, ensuring consistent text handling across platforms.
Also applies to: 280-280, 303-303, 325-325, 361-361, 364-364, 385-385, 415-415
459-461: LGTM! More specific exception type improves error handling.Raising
FileNotFoundErrorinstead of genericExceptionis more specific and pythonic, making error handling clearer for callers.vtr_flow/scripts/noc/noc_benchmark_test.py (3)
253-280: LGTM! Context manager and encoding improvements.The use of a context manager ensures proper file closure, and explicit UTF-8 encoding standardizes text handling. The indentation adjustments maintain readability.
533-535: LGTM! Context manager usage is safer.Using a context manager ensures the file is properly closed even if the subprocess fails.
632-639: LGTM! Context manager and encoding standardization.vtr_flow/scripts/python_libs/vtr/task.py (1)
392-407: LGTM! Clean refactor to usemax().The refactor from conditional assignment to using
max()is more concise and idiomatic. The logic remains functionally equivalent while improving readability.dev/pylint_check.py (1)
211-214: LGTM! Updated ignored check for pylint 4.0.4.The change from ignoring C0330 (bad-continuation) to C0209 (consider-using-f-string) aligns with the pylint version upgrade. The C0330 check was removed in newer pylint versions, and deferring f-string migration is a reasonable pragmatic choice for this PR.
vtr_flow/scripts/run_vtr_task.py (2)
408-411: LGTM! Proper UTF-8 encoding added for cross-platform compatibility.Explicitly specifying
encoding="utf-8"for file operations is good practice and ensures consistent behaviour across different platforms, particularly Windows where the default encoding can differ.
489-500: LGTM! Consistent UTF-8 encoding for log file operations.The UTF-8 encoding specification is correctly applied to both write and read operations on the same file, ensuring consistency.
vtr_flow/scripts/python_libs/vtr/parse_vtr_task.py (2)
225-240: LGTM! UTF-8 encoding properly added to parse file operations.The explicit UTF-8 encoding is correctly applied to all parse result file write operations, ensuring consistent Unicode handling across platforms.
252-264: LGTM! Consistent UTF-8 encoding for file I/O.The encoding specification is properly applied to both read and write operations throughout the function.
.github/workflows/lint.yml (1)
30-45: LGTM! Well-structured Python linting CI job.The new PythonLint job is properly configured with:
- Appropriate runner (ubuntu-24.04) aligning with the PR's goal to support the newer Ubuntu version
- Reasonable timeout (5 minutes)
- Correct dependency installation from requirements.txt
- Proper execution of the linting script
This successfully separates Python linting into a dedicated CI job.
requirements.txt (1)
10-10: Version 4.0.4 exists and is stable with no known critical security advisories.Pylint 4.0.4 is available on PyPI and has been released with proper attestations. The upgrade from pylint 2.7.4 to 4.0.4 represents a legitimate version progression through stable releases. Based on comprehensive vulnerability database searches, no critical security advisories have been identified for pylint 4.0.4; historical vulnerabilities in earlier versions (2.5.0, 2.7.0) have long been remediated in this current release.
| with request.urlopen(url) as response: | ||
| latest_package_url = response.read().decode("utf-8") | ||
| print("Downloading latest package:\n{}".format(latest_package_url)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the file
find . -name "download_symbiflow.py" -type f
# Then verify BASE_URL and GCS_URL usage
rg -n "BASE_URL|GCS_URL" vtr_flow/scripts/download_symbiflow.py -A2Repository: verilog-to-routing/vtr-verilog-to-routing
Length of output: 407
🏁 Script executed:
#!/bin/bash
# Check for any hardcoded URLs in the file
rg -n "http|file:|url" vtr_flow/scripts/download_symbiflow.py -iRepository: verilog-to-routing/vtr-verilog-to-routing
Length of output: 1248
🏁 Script executed:
#!/bin/bash
# Read the context around lines 115-117 and related URL construction
cat -n vtr_flow/scripts/download_symbiflow.py | head -120 | tail -30Repository: verilog-to-routing/vtr-verilog-to-routing
Length of output: 1443
Good improvement to use context manager for resource safety.
The switch to a context manager ensures proper resource cleanup. The .decode("utf-8") is also explicit and correct. All hardcoded URL constants (BASE_URL and GCS_URL) use HTTPS schemes.
However, the latest_package_url read from the response on line 116 is used directly with request.urlretrieve() on line 118 without validating its scheme. Consider validating that the URL from the response uses HTTPS before retrieval to prevent potential attacks through response manipulation.
🧰 Tools
🪛 Ruff (0.14.8)
115-115: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
In vtr_flow/scripts/download_symbiflow.py around lines 115 to 117, the
latest_package_url read from the HTTP response is used directly later; validate
that the returned URL uses HTTPS before calling request.urlretrieve(). Parse the
latest_package_url (trim whitespace/newlines), use urllib.parse.urlparse to
check parsed.scheme == "https" (or explicitly fail if not), and on failure log
an error and exit/raise to avoid following non-HTTPS URLs. Ensure the validation
happens immediately after decoding the response and before any download
attempts.
The python linter test was in the "test" workflow when it should be in the "lint" worflow. Moved it over.
While moving it, I noticed that it was on such an old version of PyLint that it could not even run on Ubuntu 24.04's version of Python. Upgraded the version of PyLint to make the test more stable. To do this, I had to resolve many lint issues in the Python files. I ignored the consider using f-string warning since so many files use format instead. I also increased the max positional arguments since so many files use 25+ positional arguments.
Summary by CodeRabbit
Release Notes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.