Repository navigation
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the package management from Poetry/Conda to uv and hatchling, adds optional keypoint logging functionality for debugging VR hand tracking, introduces FlatBuffers schema definitions for future serialization, updates network configuration with environment-specific settings, and includes various code improvements with TODO markers for future refactoring.
Key Changes
- Build system migration: Switched from
poetry-coretohatchlingas the build backend and updated documentation to useuvinstead of Conda for package/environment management - Keypoint logging: Added optional JSON-based logging of VR hand keypoints and coordinate frames for debugging purposes (disabled by default)
- Configuration management: Introduced environment-specific YAML configs (dev/staging/prod) for network settings and updated HOST_ADDRESS across the codebase
Reviewed Changes
Copilot reviewed 15 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Migrated build backend to hatchling, added flatbuffers dependency with invalid syntax, specified transformers version, added dev dependencies (debugpy, jupyterlab, ipykernel) |
| poetry.lock | Updated lock file with flatbuffers package and additional greenlet platform wheels |
| README.md | Replaced Conda installation instructions with uv-based setup, fixed minor typo, added build tools prerequisites |
| CONTRIBUTING.md | Updated development environment setup to use uv instead of Conda/Poetry, though pip alternative still references uv |
| .pre-commit-config.yaml | Updated ruff to v0.14.5, pre-commit-hooks to v6.0.0 (version may not exist), added max file size limit |
| src/beavr/schemas/teleop.fbs | Added new FlatBuffers schema file defining VR input, coordinate frames, and robot state structures (not yet integrated into codebase) |
| src/beavr/teleop/components/detector/vr/log_keypoints.py | New file implementing KeypointLogger class with auto-save, signal handling, and JSON serialization for debugging |
| src/beavr/teleop/components/detector/vr/keypoint_transform.py | Integrated optional logging, improved variable naming (transformed_keypoints/coordinate_frame), enhanced docstrings, added cleanup for logger |
| src/beavr/teleop/components/detector/vr/oculus.py | Added TODO comments about publishing optimization and cleanup improvements |
| src/beavr/teleop/configs/robots/shared_components.py | Disabled keypoint logging by default in TransformHandPositionCoordsCfg |
| src/beavr/teleop/configs/constants/network.py | Updated HOST_ADDRESS to 10.0.0.51 with sync warning comment |
| configs/environment/dev.yaml | Updated HOST_ADDRESS to match network.py constant |
| configs/environment/staging.yaml | New staging environment configuration file |
| configs/environment/prod.yaml | New production environment configuration file |
| src/beavr/teleop/components/operator/robots/xarm7_operator.py | Added TODO comments about removing topic literals and using constants |
| src/beavr/teleop/components/operator/robots/leap_operator.py | Added incorrect TODO suggesting subscriber removal, added whitespace formatting |
| src/beavr/teleop/components/interface/robots/xarm7_robot.py | Added TODO about external adapter data updates |
| src/beavr/teleop/components/interface/robots/leap_robot.py | Added TODO about external adapter data updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Alternatively, using pip: | ||
|
|
||
| ```bash | ||
| poetry install --with dev | ||
| # Install Python 3.10.13 | ||
| uv python install 3.10.13 | ||
|
|
||
| # Create virtual environment | ||
| uv venv --python 3.10.13 | ||
|
|
||
| # Activate the virtual environment | ||
| source .venv/bin/activate # On Linux/Mac | ||
|
|
||
| # Install dependencies | ||
| pip install -e .[dev] | ||
| ``` |
There was a problem hiding this comment.
The "Alternatively, using pip" section still requires uv to install Python and create the virtual environment (lines 120, 123). This is contradictory - if someone wants to use pip as an alternative to uv, they likely don't have uv installed. Consider providing instructions for using pip without uv, such as using system Python or pyenv to manage Python versions.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| # NOTE: Keep this in sync with your active environment config (e.g. configs/environment/dev.yaml) | ||
| HOST_ADDRESS = "10.0.0.51" |
There was a problem hiding this comment.
The hardcoded IP address "10.0.0.51" has been changed in both network.py and dev.yaml, but the comment on line 6 notes these should be kept in sync. This creates a maintenance burden where developers must manually ensure consistency between the constant file and the environment config. Consider loading HOST_ADDRESS from the environment configuration at runtime instead of hardcoding it in both places, or add validation to detect when they're out of sync.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 15 out of 19 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/beavr/teleop/components/detector/vr/keypoint_transform.py:282
- The code publishes the same data twice to different ports (lines 265-271 and 277-282). This redundancy should be addressed by migrating consumers to use a single publication point to reduce network overhead and maintain a single source of truth.
# This is redundant
# TODO: Remove this and modify LEAPoperator to use coords topic
self.publisher_manager.publish(
host=self.host,
port=self.keypoint_transform_pub_port,
topic=self.frame_topic,
data=data,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 15 out of 19 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/beavr/teleop/components/operator/robots/leap_operator.py:66
- The TODO states this subscriber is not used, but it's actually required by the abstract base class
Operator(seeoperator_base.py:31-32). WhileLeapHandOperatordoesn't directly use it, removing it would break the interface contract. If you want to remove it:
- Update the
Operatorabstract base class to make this property optional - Update the
cleanup()method inoperator_base.pyto handle cases where the property is None - Consider whether other operators (like
XArmOperator,TemplateArmOperator) that DO use this subscriber would be affected
# TODO: This subscriber is not used, remove it.
self._transformed_arm_keypoint_subscriber = ZMQSubscriber(
host=self._host,
port=self._port,
topic=frame_topic,
context=self._context,
message_type=InputFrame,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What this does
Explain what this PR does. Feel free to tag your PR with the appropriate label(s).
Examples:
How it was tested
Explain or show how you tested your changes.
Examples:
test_somethingintests/test_components.py.new_featureand verified it runs withpython -m beavr.teleop.main.some_function, confirmed it now runs X% faster.--robot_name=xarm7and confirmed it saves in the right format.How to checkout & try? (for the reviewer)
Provide a simple way for the reviewer to try out your changes.
Examples:
SECTION TO REMOVE BEFORE SUBMITTING YOUR PR
Note: Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR. Try to avoid tagging more than 3 people.
Note: Before submitting this PR, please read the contributing guidelines