feat(pypi): support importing uv.lock file#3785
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for uv.lock files in rules_python, introducing a toml2json conversion utility and updating the lock rule and requirement parsing logic. The review identifies critical compatibility issues, specifically the toml2json tool's reliance on Python 3.11's tomllib and missing serialization for date/time objects. Furthermore, the feedback highlights logic errors in how package extras are handled—which could lead to dependency bloat or failed consistency checks—and suggests improvements for platform resolution and path handling in shell scripts.
| """Parse requirements using uv.lock as the primary source.""" | ||
| ret = _parse_uv_lock_json( | ||
| uv_lock_json = uv_lock_json, | ||
| all_platforms = _get_all_platforms(requirements_by_platform) if requirements_by_platform else [], |
There was a problem hiding this comment.
If requirements_by_platform is empty (which is common when using uv.lock as the primary source), all_platforms will be an empty list. This results in all packages having empty target_platforms, which will likely cause issues in downstream rules that expect platform information for wheel selection.
| all_platforms = _get_all_platforms(requirements_by_platform) if requirements_by_platform else [], | |
| all_platforms = _get_all_platforms(requirements_by_platform) if requirements_by_platform else sorted(platforms.keys()), |
| for extra in pkg.get("provides-extras", pkg.get("extras", [])): | ||
| if extra not in entry["extras"]: | ||
| entry["extras"][extra] = None |
There was a problem hiding this comment.
Including all provides-extras in the requirement line for every package is likely incorrect. provides-extras lists all extras a package defines, not necessarily what was resolved or requested. Including all of them will force the installation of all optional dependencies for every package in the lock file, leading to significant dependency bloat. It might be better to omit extras from the requirement line if the lock file already provides the specific version and URL, or only include the extras that were part of the resolution.
- tomllib: try/except fallback to tomli for Python <3.11 - json_serializer: add datetime.date and datetime.time support - all_platforms: use sorted(platforms.keys()) fallback - $PWD/ path: check if python_path is absolute - extra_pip_args: pass through to _parse_uv_lock_json - Add uv_lock tests: multiple packages, extra_pip_args, multi-os - Update plan.md with review cycle instructions
|
Addressed all review comments. Please re-review when you get a chance. @gemini-code-assist |
There was a problem hiding this comment.
Code Review
This pull request introduces support for uv.lock files in rules_python, adding a new lock_format attribute to the lock rule and a toml2json utility for parsing. It also implements a consistency check between uv.lock and requirements files. The reviewer identified a potential runtime error in the uv.lock parser due to unsafe dictionary access for optional hash keys and suggested simplifying redundant logic during hash extraction.
- Add python_3_14 toolchain in MODULE.bazel - Always use Python 3.14 interpreter for uv.lock JSON conversion - Remove tomli fallback from toml2json.py (Python 3.14 has tomllib) - Simplify convert_uv_lock_to_json to use fixed interpreter
- Add bazel-in-bazel integration test under tests/integration/uv_lock/ - Test converts uv.lock TOML to JSON using toml2json tool - Remove dead uv_pypi test data (placeholder only) - Restore tomli fallback in toml2json.py for backward compatibility
|
Addressed all review comments and added Python 3.14 toolchain + integration test. Please re-review. @gemini-code-assist |
There was a problem hiding this comment.
Code Review
This pull request introduces support for uv.lock files as a primary metadata source in rules_python. Key additions include a toml2json tool for parsing, a new uv_lock format for the lock rule, and consistency checks within parse_requirements. Reviewer feedback suggests adopting more idiomatic Starlark dictionary access, implementing safer hash prefix stripping, and improving the portability of shell commands by replacing hardcoded environment variables with Bazel-provided paths.
|
The latest release of toml.bzl makes bazel_lib a dev dependency. It now uses skylib for the |
Remove the old toml2json Python tool and uv_lock.bzl in favor of the pure Starlark toml.bzl decoder. Update tests to pass toml_decode mock, remove is_rules_python_root references, and fix virtual package test expectations. Clean up BUILD.bazel files that referenced deleted targets.
| ), | ||
| logger = logger, | ||
| ), | ||
| uv_lock = pip_attr.uv_lock, |
There was a problem hiding this comment.
Add a test to ensure this is tested.
… lock support Remove the lock_format attribute and detect whether to use uv lock or uv pip compile from the output file extension (.lock = uv lock, else requirements). Add a Windows bat template for uv lock support. The python interpreter is passed through --python flag consistently.
rickeylev
left a comment
rickeylev
left a comment
There was a problem hiding this comment.
Overall LGTM. Some minor questions and nits. But overall, this fit in rather nicely with the pipstar code you've written, nice!
|
ai analysis of ci failure: path instead short_path should be used. I think that's right? The logic has moved around a bit, but it looks like the is_windows branch of building the args previously did e.g. |
Windows action execution was failing because the embedded paths in the generated .bat files used 'short_path' (e.g., '../repo/file'), which is relative to the runfiles directory. In the action execution environment (execroot), these paths must be relative to the execroot (e.g., 'external/repo/file'). Resolved by using 'path' instead of 'short_path' for Windows action arguments in lock.bzl.
Fix Windows test assertions to support batch script syntax and preserve forward slashes in uv arguments to ensure cross-platform lockfile path consistency.
Fix Windows batch templates to return the actual command exit code and add a temporary unified diff test to capture generated vs expected requirements mismatches on Windows CI.
…bilize them. This prevents the tests from resolving newer versions from PyPI on platforms with network access, ensuring reproducibility across all CI runners.
This resolves issues where the output file path was evaluated as empty and exit codes were masked inside parentheses. Also reverts temporary debug prints.
3 participants
Part of this is vibe coded, but I thought that the approach might have been rigorous
enough to submit a PR.
The strategy was:
uv.lockfile from thelockrule.uv.lockfile together with the requirements fileand verify things are OK.
Extra things that we could do:
uv.lockscenarios and ensure parity withrequirements.txtfiles.PyPIindex to understand if the packages are yanked or not - lockfile does not have that information.
pyproject.tomlfile to get the index values for each package.Summary:
Closes #3557
Work towards #2787