Repository navigation
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds Ruff's perflint performance rules to improve code efficiency by addressing common performance anti-patterns. The changes focus on optimizing dictionary/list operations and reducing unnecessary iterations.
- Replaced
dict.items()withdict.values()when keys aren't used - Converted manual list building loops to list comprehensions
- Added perflint rules to the linting configuration
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Added PERF rules to Ruff configuration with selective ignores |
| src/beavr/teleop/components/operator/solvers/leap_solver.py | Added noqa comment for dictionary assignment in loop |
| src/beavr/lerobot/configs/policies.py | Optimized dictionary iteration by using .values() |
| src/beavr/lerobot/common/robot_devices/robots/mobile_manipulator.py | Replaced manual list building with list comprehensions |
| src/beavr/lerobot/common/robot_devices/robots/manipulator.py | Converted loops to list comprehensions for state/action creation |
| src/beavr/lerobot/common/robot_devices/robots/beavrbot.py | Optimized dictionary iteration by using .values() |
| src/beavr/lerobot/common/robot_devices/cameras/opencv.py | Refactored camera finding logic with list comprehensions |
| src/beavr/lerobot/common/policies/smolvla/smolvlm_with_expert.py | Replaced append loop with list.extend() and generator expression |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| print( | ||
| "Mac or Windows detected. Finding available camera indices through " | ||
| f"scanning all indices from 0 to {MAX_OPENCV_INDEX}" | ||
| ) | ||
| possible_indices = range(max_index_search_range) | ||
| indices = _find_cameras(possible_indices, mock=mock) | ||
| return [ | ||
| { | ||
| "port": None, | ||
| "index": index, | ||
| } | ||
| for index in indices | ||
| ] |
There was a problem hiding this comment.
The Linux branch returns early, but the non-Linux code now runs unconditionally. This should be wrapped in an else block to maintain the original behavior.
There was a problem hiding this comment.
That would violate ruff rule RET505.
The
elsestatement is not needed as thereturnstatement will always break out of the enclosing function. Removing theelsewill reduce nesting and make the code more readable.
| # TODO: Uncomment rules when ready to use | ||
| select = [ | ||
| "E", "W", "F", "I", "B", "C4", "T20", "N" # "SIM", "A", "S", "D", "RUF", "UP" | ||
| "ASYNC", "B", "C4", "E", "F", "I", "N", "PERF", "T20", "W" # "SIM", "A", "S", "D", "RUF", "UP" |
There was a problem hiding this comment.
Keep these sorted to make it easy to spot missing rules and nearly impossible to add duplicates.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def find_cameras(raise_when_empty=False, max_index_search_range=MAX_OPENCV_INDEX, mock=False) -> list[dict]: | ||
| cameras = [] | ||
| if platform.system() == "Linux": |
There was a problem hiding this comment.
The function signature indicates it returns list[dict] but the implementation has two different return paths that may not execute. After the Linux block, there's no else clause, so the Mac/Windows code will always execute regardless of platform, potentially causing the Linux case to return twice or fall through incorrectly.
There was a problem hiding this comment.
Both possible paths return a list[dict]. The fall through is correct because Mac/Windows code will never execute in the Linux case and the Linux code will never execute in the Mac/Windows case. Returning twice is not possible in Python code because the first return leaves the function.
f01fd96 to
a59f59d
Compare
a59f59d to
42025e0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if platform.system() == "Linux": | ||
| print("Linux detected. Finding available camera indices through scanning '/dev/video*' ports") | ||
| possible_ports = [str(port) for port in Path("/dev").glob("video*")] | ||
| ports = _find_cameras(possible_ports, mock=mock) |
There was a problem hiding this comment.
The refactoring removed the early return logic and changed the control flow. The raise_when_empty parameter is no longer being passed to the _find_cameras function calls. The original code had implicit fall-through to the else block, but now when the Linux branch returns early, the function doesn't handle the raise_when_empty parameter for either path.
| f"scanning all indices from 0 to {MAX_OPENCV_INDEX}" | ||
| ) | ||
| possible_indices = range(max_index_search_range) | ||
| indices = _find_cameras(possible_indices, mock=mock) |
There was a problem hiding this comment.
The refactoring removed the early return logic and changed the control flow. The raise_when_empty parameter is no longer being passed to the _find_cameras function call. The original code passed this parameter through, but now it's missing.
What this does
perflint: Add ruff rules for performance -- https://docs.astral.sh/ruff/rules/#perflint-perf
How it was tested
%
pre-commit run --all-files%
ruff rule PERF401manual-list-comprehension (PERF401)
Derived from the Perflint linter.
Fix is sometimes available.
What it does
Checks for
forloops that can be replaced by a list comprehension.Why is this bad?
When creating a transformed list from an existing list using a for-loop,
prefer a list comprehension. List comprehensions are more readable and
more performant.
Using the below as an example, the list comprehension is ~10% faster on
Python 3.11, and ~25% faster on Python 3.10.
Note that, as with all
perflintrules, this is only intended as amicro-optimization, and will have a negligible impact on performance in
most cases.
Example
Use instead:
If you're appending to an existing list, use the
extendmethod instead: