Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates core platform utilities and management workflows by improving version detection, adding a library removal path (server RPC + CLI), and making wheel filename parsing more robust.
Changes:
- Update dev-mode version lookup to read
pyproject.tomlfrom repo root and parse TOML viatomli. - Add
remove_librarysupport via ControlService RPC, AIP implementation, and a newvctl remove-libcommand. - Improve wheel filename parsing to handle both
py3andpy2.py3compatible wheels, with explicit erroring on unexpected formats.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/volttron/utils/version.py |
Adjusts repo-root detection and switches TOML parsing implementation for dev-mode version lookup. |
src/volttron/services/control/control_service.py |
Exposes a new RPC endpoint to remove an installed library via the server AIP. |
src/volttron/server/aip.py |
Implements poetry remove for libraries and updates wheel filename parsing logic. |
src/volttron/client/commands/control.py |
Adds a new remove-lib CLI subcommand that calls the server RPC. |
pyproject.toml |
Removes the toml dependency (but needs alignment with the new TOML parser choice). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/volttron/utils/version.py
Outdated
| root = Path(__file__).parent.parent.parent.parent | ||
| tomle_file = root.joinpath("pyproject.toml") | ||
| if not tomle_file.exists(): |
There was a problem hiding this comment.
Variable name tomle_file looks like a typo (likely meant toml_file). Renaming would improve readability and avoid propagating the misspelling through the error message and subsequent uses.
| [tool.poetry.dependencies] | ||
| poetry = "^2.0" | ||
| python = "^3.10" | ||
| gevent = ">=24.2.1,<24.3" | ||
| PyYAML = "^6.0" | ||
| toml = "^0.10.2" | ||
| dateutils = "^0.6.12" | ||
| tzlocal = "^4.1" | ||
| psutil = "^5.9.0" |
There was a problem hiding this comment.
This PR removes the toml dependency, but the code now reads pyproject.toml via tomli (see src/volttron/utils/version.py). Add tomli (ideally with a Python version marker like <3.11) or switch the implementation to stdlib tomllib so get_version() works in developer mode.
| pre_release=allow_prerelease) | ||
|
|
||
| @RPC.export | ||
| def remove_library(self, name): |
There was a problem hiding this comment.
remove_library is exported over RPC but doesn't follow the parameter type validation pattern used by other RPC methods in this file (e.g., remove_agent, stop_agent validate inputs and include caller identity in the error). Add a str type check (and consistent error message) to avoid unexpected tracebacks from bad RPC inputs.
| def remove_library(self, name): | |
| def remove_library(self, name): | |
| # Validate parameter type and include caller identity in error, to match | |
| # the pattern used by other RPC methods in this service. | |
| get_identity = getattr(cc, "get_current_identity", None) | |
| if callable(get_identity): | |
| caller_identity = get_identity() | |
| else: | |
| caller_identity = getattr(cc, "current_identity", "unknown") | |
| if not isinstance(name, str): | |
| raise ValueError( | |
| f"Invalid library name {name!r} (type {type(name).__name__}) from caller {caller_identity}" | |
| ) |
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 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/volttron/utils/version.py
Outdated
| __version__ = importlib_metadata.version('volttron-core') | ||
|
|
||
| except importlib_metadata.PackageNotFoundError: | ||
| # TODO - use tomlib once we migrate to py 3.11 or above |
There was a problem hiding this comment.
The TODO references tomlib, but the stdlib module name is tomllib (Python 3.11+). Updating the comment avoids confusion and aligns with the actual migration target.
| # TODO - use tomlib once we migrate to py 3.11 or above | |
| # TODO - use tomllib once we migrate to py 3.11 or above |
| root = Path(__file__).parent.parent.parent.parent | ||
| toml_file = root.joinpath("pyproject.toml") | ||
| if not toml_file.exists(): | ||
| raise ValueError( | ||
| f"Couldn't find pyproject.toml file for finding version. ({str(tomle_file)})") | ||
| import toml | ||
| f"Couldn't find pyproject.toml file for finding version. ({str(toml_file)})") | ||
|
|
||
| pyproject = toml.load(tomle_file) | ||
| # Prefer stdlib tomllib when available (Python 3.11+), fall back to tomli if installed. | ||
| try: |
There was a problem hiding this comment.
tomli is only installed for Python <3.11 (per pyproject), but python = ^3.10 allows running this code on 3.11+. In a dev checkout on 3.11+, this branch will raise ModuleNotFoundError when volttron-core metadata isn't present. Consider importing tomllib when available and falling back to tomli (or adjust dependencies accordingly).
| # handle both py3 and python2&3 compatible wheels | ||
| stripped = re.sub(r'-(py2\.py3|py3)-[^-]+-[^-]+\.whl$', '', wheel) | ||
| if stripped == wheel: | ||
| raise RuntimeError( | ||
| f"Unable to parse package name from wheel filename '{wheel}'. " | ||
| f"Expected format: {{name}}-{{version}}-(py3|py2.py3)-{{abi}}-{{platform}}.whl" | ||
| ) |
There was a problem hiding this comment.
The wheel filename parser only accepts python tags py3 or py2.py3 and will raise for valid wheels like pkg-1.0.0-cp310-cp310-manylinux_2_17_x86_64.whl (and also doesn’t account for the optional PEP 427 build tag). Since install_agent_or_lib_source can receive arbitrary wheels, consider parsing per PEP 427 more generally (e.g., accept any python/abi/platform tags and handle an optional build tag) so compiled/tagged wheels don’t get rejected.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # handle both py3 and python2&3 compatible wheels | ||
| stripped = re.sub(r'-(py2\.py3|py3)-[^-]+-[^-]+\.whl$', '', wheel) | ||
| if stripped == wheel: | ||
| raise RuntimeError( | ||
| f"Unable to parse package name from wheel filename '{wheel}'. " | ||
| f"Expected format: {{name}}-{{version}}-(py3|py2.py3)-{{abi}}-{{platform}}.whl" | ||
| ) | ||
| agent_name_with_version = stripped.replace("_", "-") |
There was a problem hiding this comment.
_construct_package_name_from_wheel now only accepts wheel filenames with a py3 or py2.py3 Python tag and raises for other valid wheel tags (e.g., py310, cp310, etc.). Since install_agent_or_lib_source calls this for any .whl, this can break installing perfectly valid wheels. Consider using a standards-based wheel filename parser (or expanding the parsing to cover PEP 427 wheel tags and optional build tags) rather than hard-coding py3|py2.py3 and erroring out.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.