Conversation
github-actions
Bot
added
pr/external
stage/needs-triage
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
|
Thanks for the fix and the unusually clear write-up, @noritada — and apologies for the long silence on this one. I reproduced this end to end and your diagnosis is correct. Details below, including one thing I'd like changed before merge. Yes, this fixes #892I built the exact workspace from your repro (bare root +
So #892 is closable when this merges. I also confirmed that both halves of the change are load-bearing, which wasn't obvious to me from the diff alone. With only the And your claim about export paths (astral-sh/uv#20238) checks out: exporting from the member dir emits Your approach also looks like the only reliable one here. Because uv writes One change requested: fall back when
|
There was a problem hiding this comment.
Code Review Results
Reviewed: 37cd1d1..b813ed8
Files: 9
Comments: 2
Comments on lines outside the diff:
[aws_lambda_builders/workflows/python_uv/packager.py:372] [BUG] uv export still runs with cwd=project_dir while uv pip install now runs with cwd=workspace_dir, so any relative path shared by the two commands no longer resolves to the same location.
temp_requirements is os.path.join(scratch_dir, "lock_requirements.txt"), and scratch_dir is never absolutized on the way in — PythonUvBuildAction forwards whatever the workflow was given. With a relative scratch dir, --output-file is written under project_dir but -r is looked up under the workspace root, so the install fails with a missing requirements file for exactly the workspace layout this PR fixes. install_requirements already guards --target against this (and its comment notes the incremental-build dependencies dir does arrive relative in practice), so the requirements path deserves the same treatment:
args.extend(["-r", os.path.abspath(requirements_path)])config.cache_dir is derived from scratch_dir too and is passed to uv as --cache-dir from the new cwd, so it has the same exposure.
| python_version=python_version, | ||
| platform="linux", | ||
| cwd=project_dir, | ||
| cwd=workspace_dir, |
There was a problem hiding this comment.
[BUG] Splitting the two UV invocations across different working directories breaks the relative path they share.
temp_requirements is built as os.path.join(scratch_dir, "lock_requirements.txt") and is never absolutized. It is written by uv export --output-file (which still runs with cwd=project_dir) and then read by uv pip install -r (which now runs with cwd=workspace_dir). Before this change both commands shared the same cwd, so a relative scratch_dir resolved consistently. Now, when workspace_dir != project_dir, export writes to project_dir/<scratch_dir>/lock_requirements.txt while install looks for workspace_dir/<scratch_dir>/lock_requirements.txt and fails. config.cache_dir (set to os.path.join(scratch_dir, "uv-cache") in _ensure_cache_dir) has the same problem and would silently create a second cache directory.
scratch_dir is not normalized anywhere in the call chain — builder.py only does os.makedirs(scratch_dir) on it. This codebase already treats these paths as possibly relative: install_requirements deliberately wraps the target in os.path.abspath() (line 142, with a comment about UV's cwd, covered by test_install_requirements_resolves_relative_target_to_absolute), and java_gradle/actions.py:55 applies os.path.abspath to scratch_dir for the same reason.
Anchor the shared path so it is independent of which cwd UV runs in:
temp_requirements = os.path.abspath(os.path.join(scratch_dir, "lock_requirements.txt"))Absolutizing requirements_path inside install_requirements (mirroring the existing --target handling) would also protect the _build_from_requirements path, which passes a caller-supplied manifest path.
Note: this was raised in the previous review round at packager.py:372 and does not appear to have been addressed or explicitly dismissed, so re-raising.
There was a problem hiding this comment.
Code Review Results
Reviewed: 37cd1d1..45b8529
Files: 9
Comments: 3
Comments on lines outside the diff:
[tests/unit/workflows/python_uv/test_packager.py:126] [BUG] This existing assertion was not updated for the new abspath behavior and will fail on Windows.
install_requirements now passes os.path.abspath(requirements_path) (packager.py:137), and on Windows os.path.abspath("/path/to/requirements.txt") returns C:\path\to\requirements.txt (drive letter added, separators flipped). The literal "/path/to/requirements.txt" is therefore no longer an element of args_called, so assertIn raises. Unit tests run on windows-latest across the whole Python matrix in .github/workflows/build.yml, so this breaks half of CI.
The neighbouring test_install_requirements_resolves_relative_target_to_absolute already shows the correct pattern — compare against the absolutized value:
self.assertEqual(
args_called[args_called.index("-r") + 1],
os.path.abspath("/path/to/requirements.txt"),
)| # Get the workspace root (or project directory if no workspace is used) | ||
| # For packages in the workspace, exported paths are relative to the workspace root, | ||
| # regardless of where in the workspace uv export is called | ||
| workspace_args = ["workspace", "dir"] |
There was a problem hiding this comment.
[GENERAL] Now that the workspace root is discoverable, _handle_pyproject_build's uv.lock probe is wrong for the exact scenario this PR adds support for. It checks os.path.join(manifest_dir, "uv.lock"), but a uv workspace keeps a single lockfile at the workspace root, not next to each member's pyproject.toml. The new integration test confirms this: it asserts uv.lock exists at workspace_dir and explicitly asserts it does not exist in source_dir.
Two consequences for workspace members:
- The "use lock-based build for precise dependencies" branch is unreachable — a committed root
uv.lockis ignored and_build_from_pyprojectre-runsuv lock --python <version>on every build, which can re-resolve and rewrite the user's lockfile. That defeats the reproducibility the lock path exists to provide. uv lockwrites into the workspace root, i.e. a directory above the function's source dir. The build mutates files outside the CodeUri, and fails outright if that parent is read-only.
Resolving the workspace root before the lock lookup (rather than only between export and install) would let both the lock discovery and the install use it consistently.
…bers This commit fixes an issue where an application created as a member of a uv workspace would fail to build if they depended on other workspace members. The following shows the problematic workspace structure and the error message. ``` workspace root ├── lib │ ├── pyproject.toml │ └── src ├── pyproject.toml ├── sam-app │ ├── __init__.py │ ├── app.py │ ├── pyproject.toml │ ├── samconfig.toml │ └── template.yaml └── uv.lock ``` ``` Build Failed Error: PythonUvBuilder:ResolveDependencies - UV package build failed: Failed to build from pyproject.toml: Lock file operation failed: Failed to install dependencies using uv: UV pip install failed: Using CPython 3.13.0 interpreter at: /path/to/workspace/.venv/bin/python3 error: Distribution not found at: file:///path/to/workspace/sam-app/lib ``` Even though `lib` and `sam-app` are in the same directory level in the workspace, the workflow attempts to install `lib` under `sam-app`. In the workflow, `uv export` outputs a list of dependency packages, which is then passed to `uv pip install` for installation. When doing so, [`uv export` outputs relative paths from the workspace root][1]. Therefore, `uv pip install` must be run from the workspace root, not from the application directory. Additionally, dependencies on other packages within the workspace are exported as editable installations (e.g., `-e ./lib`) by default. When this is passed to `uv pip install`, only the `.pth` (path configuration) file for the package will be installed without the package body. To prevent this, the `--no-editable` option needs to be used. [1]: astral-sh/uv#20238
| # Get the workspace root (or project directory if no workspace is used) | ||
| # For packages in the workspace, exported paths are relative to the workspace root, | ||
| # regardless of where in the workspace uv export is called | ||
| workspace_args = ["workspace", "dir"] |
There was a problem hiding this comment.
[GENERAL] Now that the workspace root is discoverable here, the uv.lock probe in _handle_pyproject_build is wrong for exactly the scenario this PR adds support for. It checks os.path.join(manifest_dir, "uv.lock"), but a uv workspace keeps a single lockfile at the workspace root, not next to each member's pyproject.toml. The new integration test confirms this — it asserts uv.lock exists at workspace_dir and explicitly does not exist at source_dir.
Consequences for a workspace member that already has a committed lockfile at the root:
os.path.existsreturnsFalse, so the build always falls through to_build_from_pyprojectand re-runsuv lock --python <version>, defeating the "use lock-based build for precise dependencies" behavior.- That
uv lockwrites to the workspace root, i.e. it mutates a file outside the function's source directory, and can change pins the user deliberately committed. - Re-resolving requires network access, so a build that would have succeeded from a complete lockfile can now fail.
lock_path then points at a file that does not exist (os.path.join(project_dir, "uv.lock") in _build_from_pyproject); it happens to work only because _build_from_lock_file uses it solely for os.path.dirname. Worth resolving the workspace root before probing for the lockfile (the same uv workspace dir lookup added here), and adding an integration case with a pre-existing workspace-root uv.lock — the new test only covers the no-lockfile path.
… and `uv pip install`
| # Get the workspace root (or project directory if no workspace is used) | ||
| # For packages in the workspace, exported paths are relative to the workspace root, | ||
| # regardless of where in the workspace uv export is called | ||
| workspace_args = ["workspace", "dir"] |
There was a problem hiding this comment.
[GENERAL] The workspace root is now discoverable here, but the uv.lock probe in _handle_pyproject_build (packager.py:267-270) still assumes the lockfile sits next to pyproject.toml — which is never true for the layout this PR adds support for, since uv keeps a single lockfile at the workspace root. The new integration test documents this directly: it asserts uv.lock exists at workspace_dir and asserts it does not exist in source_dir.
Consequences when building a workspace member:
os.path.exists(os.path.join(manifest_dir, "uv.lock"))is alwaysFalse, so the build takes_build_from_pyprojectand re-runsuv lockinstead of the "lock-based build for precise dependencies" path — even when the user has a committed lockfile at the workspace root.- The
python_uvworkflow runs uv against the real source tree (it passessource_dir/manifest_pathstraight through;CopySourceActionruns last), so thatuv lockwrites or refreshesuv.lockin the user's workspace root — a directory outside the function's source dir. lock_path = os.path.join(project_dir, "uv.lock")in_build_from_pyproject(packager.py:381) then names a file that does not exist. It is harmless only because_build_from_lock_fileuses it solely to deriveproject_dir, which makes it an easy trap for the next change.
Since this hunk already resolves the workspace root, consider extracting it into a helper and using it for the lock probe as well, so a member with a root lockfile takes the lock-based path:
def _resolve_workspace_dir(self, project_dir: str) -> str:
rc, stdout, stderr = self._uv_runner._uv.run_uv_command(["workspace", "dir"], cwd=project_dir)
if rc == 0:
return stdout.strip()
LOG.debug("Could not determine workspace root, assuming no workspace: %s", stderr)
return project_dir_handle_pyproject_build can then check manifest_dir first and fall back to os.path.join(self._resolve_workspace_dir(manifest_dir), "uv.lock").
The new integration fixture only covers the lock-less case, so this path stays untested — a fixture with a committed workspace-root uv.lock would cover it.
| """Find the workspace root, falling back for UV versions without this command.""" | ||
| rc, stdout, stderr = self._uv_runner._uv.run_uv_command(["workspace", "dir"], cwd=project_dir) | ||
| if rc == 0: | ||
| return stdout.strip() |
There was a problem hiding this comment.
[BUG] stdout is returned and used directly as a filesystem path without normalizing its type, and the existing functional tests feed it bytes. Three tests in tests/functional/workflows/python_uv/test_packager.py will fail after this change, and make pr runs tests/functional:
test_can_build_pyproject_dependencies—fake_uv.set_return_tuple(0, b"Resolved 2 packages", b"")test_can_build_with_lock_file_optimization—fake_uv.set_return_tuple(0, b"Using existing lock file", b"")test_can_handle_large_dependency_trees—set_return_tuple(0, f"Resolved {len(reqs)} packages".encode(), b"")
All three use a pyproject.toml manifest, so they now reach _resolve_workspace_dir. FakeUv.run_uv_command returns its canned tuple for every subcommand, so rc == 0 and stdout.strip() yields a bytes object. The next statement then does:
uv_lock_path = os.path.join(workspace_dir, "uv.lock") # workspace_dir is byteswhich raises TypeError: Can't mix strings and bytes in path components. _handle_pyproject_build has no try, so the error propagates out of build_dependencies and the tests fail rather than exercising the intended paths.
The fake is the unrealistic part here — OSUtils.run_subprocess uses text=True, so real stdout is always str. The minimal fix is to update those three fixtures to return str (they should arguably not have been bytes to begin with). Separately, consider guarding against a success exit with no usable output, since an empty stdout would silently produce workspace_dir = "" and then cwd="" for uv pip install:
rc, stdout, stderr = self._uv_runner._uv.run_uv_command(["workspace", "dir"], cwd=project_dir)
workspace_dir = stdout.strip() if rc == 0 else ""
if workspace_dir:
return workspace_dir| return stdout.strip() | ||
| # `uv workspace dir` requires uv >= 0.9.9. For a non-workspace project, | ||
| # the project directory is also what the command would return. | ||
| LOG.debug("Could not determine workspace root, assuming no workspace: %s", stderr) |
There was a problem hiding this comment.
[ERROR_HANDLING] Any non-zero exit from uv workspace dir is swallowed at DEBUG level and treated as "no workspace". The comment frames this as version compatibility, but the branch cannot distinguish an old uv from a real failure (unreadable/malformed pyproject.toml, permission error, uv unable to spawn — recall OSUtils.run_subprocess also converts exceptions into (1, "", str(e))). Two consequences, both silent:
- For a genuine workspace member, the fallback restores exactly the behavior this PR fixes:
install_requirementsruns withcwd=project_dirand the build dies with the crypticerror: Distribution not found at: file:///.../app/libfrom Bug: PythonUvBuilder fails to build app with dependencies on editable installs in the workspace #892. Sincesam builddoes not surfaceDEBUGlogs, the user gets no hint that upgradinguvresolves it. _handle_pyproject_buildthen probesuv.lockat the member directory, misses the workspace-root lockfile, and routes to_build_from_pyproject, which runsuv lockand re-resolves dependencies. A transientuvfailure therefore silently downgrades a pinned, reproducible build into a fresh resolution that can install different versions than the lockfile pins.
Raising the log level and making it actionable keeps the graceful degradation while leaving a trail:
LOG.warning(
"Could not determine the UV workspace root (uv workspace dir requires uv >= 0.9.9); "
"assuming %s is not part of a workspace. If this project is a workspace member, "
"upgrade uv. stderr: %s",
project_dir,
stderr,
)
return project_dir
| def _resolve_workspace_dir(self, project_dir: str) -> str: | ||
| """Find the workspace root, falling back for UV versions without this command.""" | ||
| rc, stdout, stderr = self._uv_runner._uv.run_uv_command(["workspace", "dir"], cwd=project_dir) | ||
| workspace_dir = stdout.strip() if rc == 0 else "" |
There was a problem hiding this comment.
[ERROR_HANDLING] _resolve_workspace_dir treats every non-zero exit from uv workspace dir as "old uv / not a workspace" and continues with project_dir. The branch cannot actually distinguish the cases it claims to, and OSUtils.run_subprocess (utils.py) also converts exceptions into (1, "", str(e)), so a failure to spawn uv at all lands here too:
rc, stdout, stderr = self._uv_runner._uv.run_uv_command(["workspace", "dir"], cwd=project_dir)
workspace_dir = stdout.strip() if rc == 0 else ""
if workspace_dir:
return workspace_dir
LOG.warning(
"Could not determine the UV workspace root (uv workspace dir requires uv >= 0.9.9); "
...
)
return project_dirConsequences for a real failure (unreadable/malformed pyproject.toml, permission error, spawn failure) on a modern uv:
- The warning asserts a cause that is wrong and tells the user to upgrade uv, which will not help.
- The build proceeds with workspace_dir == project_dir, so _handle_pyproject_build probes project_dir/uv.lock and uv pip install runs from project_dir — reproducing exactly the Distribution not found at: .../sam-app/lib failure this PR fixes, now masked behind a misleading message.
Suggest keying the fallback on evidence of an unsupported subcommand and propagating anything else, rather than assuming the cause. For example, only fall back when stderr indicates the subcommand is unrecognized (or gate on the already-available self._uv_runner.uv_version), and raise UvBuildError otherwise so the real stderr surfaces as the build error instead of a warning. At minimum, drop the "requires uv >= 0.9.9 / upgrade uv" claim from the message when the cause is unknown.
This was raised on an earlier revision; the log level was raised from DEBUG to WARNING and stderr was added, which helps, but the conflation of causes and the silent wrong-root fallback remain.
| workspace_dir = stdout.strip() if rc == 0 else "" | ||
| if workspace_dir: | ||
| return workspace_dir | ||
| unsupported_command = rc != 0 and "unrecognized subcommand 'workspace'" in stderr |
There was a problem hiding this comment.
[ERROR_HANDLING] The graceful-fallback path is gated on an exact substring of UV's stderr, while every other non-zero exit is now a hard build failure:
unsupported_command = rc != 0 and "unrecognized subcommand 'workspace'" in stderr
if not unsupported_command:
reason = stderr.strip() or f"uv workspace dir returned no workspace path (exit code {rc})"
raise UvBuildError(reason=f"Could not determine the UV workspace root for {project_dir}: {reason}")This makes an external tool's error wording load-bearing for build success. If the message differs at all for the older UV releases this branch exists to support — different phrasing, a quoted-argument variant, or a localized/wrapped message — users on those versions get Could not determine the UV workspace root instead of the intended fallback, turning a previously working build into a hard failure. Note also that OSUtils.run_subprocess (utils.py:30-31) funnels any exception into (1, "", str(e)), so spawn failures land in this same non-matching branch.
The condition you actually want to express is already available in the codebase and is even named in the warning text (requires uv >= 0.9.9): SubprocessUv.get_uv_version() / UvRunner.uv_version. Gating the fallback on the parsed version rather than on stderr text makes the "old UV" case deterministic, and leaves the hard failure for genuine errors only:
# fall back when uv predates uv workspace dir, otherwise surface the real error
if not self._supports_workspace_dir():
LOG.warning(...)
return project_dir
raise UvBuildError(...)get_uv_version() already returns None on failure, so an unparseable version can keep whichever default you prefer.
The rest of the change holds up. The path fix is coherent: temp_requirements is absolutized at creation, install_requirements absolutizes both -r and --target, and cwd is split correctly (export from project_dir to pick the member's dependency set, pip install from workspace_dir to resolve the relative local paths UV emits). Moving the uv.lock probe to the workspace root and threading project_dir/workspace_dir through _build_from_lock_file and _build_from_pyproject is consistent across both entry paths, including the old-UV fallback where the two collapse to the same directory. The earlier review rounds' findings — the relative temp_requirements shared across two working directories, the workspace-root lockfile probe, the bytes stdout in the functional fixtures, the os.path.relpath cross-drive ValueError, and the hardcoded forward-slash assertions — are all resolved in this revision.
| version = self._uv_runner.uv_version | ||
| # Only interpret plain release versions. Unknown or prerelease versions | ||
| # must prove support by successfully executing the command. | ||
| if version and re.fullmatch(r"[0-9]+\.[0-9]+\.[0-9]+", version): |
There was a problem hiding this comment.
[ERROR_HANDLING] The old-uv fallback only triggers when uv --version yields a plain X.Y.Z string. When it does not, any failure of uv workspace dir becomes a hard UvBuildError, which turns previously-working builds into failures for projects that are not workspace members at all.
get_uv_version() (packager.py:37-53) swallows every exception and returns None — on a spawn failure, a non-zero uv --version, or any output format it cannot split. It also returns the raw token, so a dev/prerelease build such as 0.9.8+dev or 0.9.0-alpha.1 fails the re.fullmatch. In all of those cases the code falls through to the probe, and uv replies unrecognized subcommand 'workspace' (rc != 0), so:
workspace_dir = stdout.strip() if rc == 0 else ""
if workspace_dir:
return workspace_dir
reason = stderr.strip() or f"uv workspace dir returned no workspace path (exit code {rc})"
raise UvBuildError(reason=f"Could not determine the UV workspace root for {project_dir}: {reason}")tests/unit/.../test_packager.py encodes this: test_resolve_workspace_dir_probes_supported_or_unknown_versions asserts that version None / "" / "0.9.9rc1" plus unrecognized subcommand 'workspace' raises. A single-project build that has nothing to do with workspaces then fails outright, even though project_dir is the correct answer for it — the pre-PR behavior.
Treating that specific stderr as the same signal as the version gate keeps the hard failure for genuine errors (unreadable pyproject.toml, permission denied) while preserving the documented degraded path:
rc, stdout, stderr = self._uv_runner._uv.run_uv_command(["workspace", "dir"], cwd=project_dir)
if rc != 0 and "unrecognized subcommand 'workspace'" in stderr:
LOG.warning(
"This UV build does not support `uv workspace dir` (requires uv >= 0.9.9); "
"falling back to project directory %s. If this project is a workspace member, upgrade uv.",
project_dir,
)
return project_dirA workspace member on such a uv still fails loudly at uv pip install with the original #892 error, so nothing is silently mis-built.
| workspace_dir = stdout.strip() if rc == 0 else "" | ||
| if workspace_dir: | ||
| return workspace_dir | ||
| if rc != 0 and "unrecognized subcommand 'workspace'" in stderr: |
There was a problem hiding this comment.
[ERROR_HANDLING] Re-raising a previously flagged, still-unresolved concern (the narrowing helps but does not close it).
The graceful fallback now has exactly two triggers: a uv --version string that parses as plain X.Y.Z and is < 0.9.9, or a stderr containing the literal substring unrecognized subcommand 'workspace'. Every other non-zero exit becomes a hard UvBuildError that aborts the build before the uv.lock probe ever runs:
rc, stdout, stderr = self._uv_runner._uv.run_uv_command(["workspace", "dir"], cwd=project_dir)
workspace_dir = stdout.strip() if rc == 0 else ""
if workspace_dir:
return workspace_dir
if rc != 0 and "unrecognized subcommand 'workspace'" in stderr:
...
return project_dir
reason = stderr.strip() or f"uv workspace dir returned no workspace path (exit code {rc})"
raise UvBuildError(reason=f"Could not determine the UV workspace root for {project_dir}: {reason}")Two paths reach the raise for a project that is not a workspace member at all and previously built fine, because _handle_pyproject_build never invoked uv for workspace resolution before this change:
get_uv_version()(packager.py:37-53) swallows every exception and returnsNone, and it only returnsparts[1]of theuv --versionoutput. When the version is unparseable, or non-plain (0.9.8+dev,0.9.0-alpha.1), the version gate is skipped by design, so the fallback rests entirely on the stderr wording.OSUtils.run_subprocess(utils.py) also converts spawn exceptions into(1, "", str(e)), so those land here too.rc == 0with empty or whitespace-only stdout also raises, even though the command reported success.
_resolve_workspace_dir is called for every pyproject.toml build, so this failure mode is not scoped to workspace users. Consider inverting the gate: fall back to project_dir on any failure the code cannot positively attribute to a real workspace-resolution error, and reserve the hard error for rc == 0 with output that is not a usable directory (or verify with os.path.isdir). I recognize the tests encode the current behavior as intentional; if that is the deliberate tradeoff, a note saying so would let this be closed out rather than re-raised.
7a81667 to
5678c10
Compare
| scratch_dir, | ||
| python_version, | ||
| architecture, | ||
| config, |
There was a problem hiding this comment.
[GENERAL] The old-uv fallback logs at WARNING on every pyproject.toml build, even when no workspace is involved.
_resolve_workspace_dir is now called unconditionally from _handle_pyproject_build, so any user on uv < 0.9.9 building a plain standalone project (no workspace root anywhere above it) will see this on every sam build:
UV 0.8.17 does not support `uv workspace dir` (requires uv >= 0.9.9); falling back to project directory /src. If this project is a workspace member, upgrade uv.
For the standalone case the fallback is exactly the pre-PR behavior and nothing is degraded, so the message is pure noise plus an upgrade recommendation the user does not need. The second fallback branch (stderr contains unrecognized subcommand 'workspace') has the same property.
Two cheap options: log at DEBUG and only escalate to WARNING when the project actually looks like it could be a member (e.g. a pyproject.toml with [tool.uv.workspace] exists in an ancestor directory), or keep the WARNING but defer it until a workspace-relative install is about to happen. The corresponding assertLogs(..., level="WARNING") expectations in tests/unit/workflows/python_uv/test_packager.py would need to follow.
Everything else I checked looks correct. Specifically, the concerns raised in earlier rounds appear addressed in this revision: temp_requirements and the -r argument are now absolutized so export (cwd=project_dir) and install (cwd=workspace_dir) agree on the path; the uv.lock probe moved to the resolved workspace root; the unit tests account for the second run_uv_command call and use os.path.join for Windows-safe comparisons; the functional fake now returns str rather than bytes; and os.path.relpath in the new integration test is guarded against the cross-drive ValueError. The hard-failure policy for unexplained uv workspace dir errors is explicitly documented as an accepted tradeoff in DESIGN.md, so I am not re-raising it.
| try: | ||
| with open(manifest, "rb") as file: | ||
| config = tomllib.load(file) | ||
| has_uv_workspace_table = "workspace" in config.get("tool", {}).get("uv", {}) |
There was a problem hiding this comment.
[ERROR_HANDLING] The exception guard in _log_workspace_fallback is narrower than the operations it protects, so a diagnostic-only helper can abort the build.
config = tomllib.load(file)
has_uv_workspace_table = "workspace" in config.get("tool", {}).get("uv", {})except (OSError, ValueError) covers I/O errors and TOMLDecodeError, but not the type errors this duck-typed access can raise on syntactically valid TOML:
tool = "text"at top level →config.get("tool", {})returns astr, then.get("uv", {})raisesAttributeError.[tool]withuv = 5→"workspace" in 5raisesTypeError: argument of type 'int' is not iterable.
The unit test added in test_workspace_hint_ignores_comments_and_invalid_or_unreadable_toml covers [tool]\nuv = "text", which survives only incidentally ("workspace" in "text" is a valid substring check), so neither shape above is exercised.
This matters more than a typical malformed-manifest case because the loop walks every ancestor directory up to the filesystem root and parses any pyproject.toml it finds — including files outside the project that the user did not author and that are never otherwise consumed by the build. An unrelated file several levels up can therefore fail a build that would otherwise succeed, and the only thing lost by skipping it is a warning hint.
Suggest either broadening the guard or type-checking the lookup:
tool = config.get("tool")
uv_config = tool.get("uv") if isinstance(tool, dict) else None
has_uv_workspace_table = isinstance(uv_config, dict) and "workspace" in uv_configOther previously raised concerns appear addressed in this revision: the export/install relative-path split is fixed by absolutizing temp_requirements and requirements_path; the uv.lock probe now uses the resolved workspace root; the functional-test fake returns str instead of bytes; the integration test guards os.path.relpath against cross-drive ValueError; and the old-uv fallback now logs at DEBUG with the WARNING gated on an actual tool.uv.workspace table. The hard-failure policy for unexplained uv workspace dir failures is explicitly documented as intentional in both the code comments and DESIGN.md, so I am not re-raising it.
|
|
||
| try: | ||
| import tomllib | ||
| except ModuleNotFoundError: # Python 3.10 |
There was a problem hiding this comment.
[GENERAL] ```python
try:
import tomllib
except ModuleNotFoundError: # Python 3.10
import tomli as tomllib
This module-level import, plus the new `tomli>=1.1.0; python_version < "3.11"` entry in `requirements/base.txt`, makes `tomli` a hard install dependency of the whole package for every consumer — but the only use of `tomllib` in this PR is `_log_workspace_fallback`, a diagnostics-only helper that emits a DEBUG/WARNING hint on uv < 0.9.9. It never influences the resolved root or the build path (as DESIGN.md states).
Two concrete consequences:
- `setup.py` builds `install_requires` from `read_requirements("base.txt") + read_requirements("python_pip.txt")`, so this lands in base rather than a workflow-scoped file. The existing `requirements/python_pip.txt` already carries a TODO about moving workflow deps out of shared requirements; this adds a new one in the opposite direction.
- The fallback `import tomli` is unguarded, so if it is ever unavailable on 3.10 the `ModuleNotFoundError` propagates out of the `except` block and `packager.py` becomes unimportable — taking down every python-uv build, including plain `requirements.txt` ones that never parse TOML.
Both go away by importing lazily inside the helper and degrading to no hint:
```python
def _log_workspace_fallback(self, project_dir: str) -> None:
LOG.debug(...)
try:
import tomllib
except ModuleNotFoundError:
try:
import tomli as tomllib
except ModuleNotFoundError:
return # hint unavailable; not worth failing the build
...
That also lets tomli be dropped from base.txt, or moved to a uv-workflow-specific requirements file if the hint is considered important enough to guarantee.
Other previously raised items check out as addressed: the export/install relative-path mismatch is fixed by absolutizing temp_requirements and requirements_path; the uv.lock probe now uses the resolved workspace root; the _log_workspace_fallback TOML access is isinstance-guarded; the functional-test fakes return str instead of bytes; and the integration test guards os.path.relpath against cross-drive ValueError. The hard-fail-on-uncertain-discovery policy is explicitly documented as accepted in DESIGN.md and in the inline comment, so I did not re-raise it.
b43ad6e to
d7a4121
Compare
| self.mock_uv_runner._uv.run_uv_command.assert_not_called() | ||
| self.assertIn(project_dir, logs.output[0]) | ||
| self.assertIn("uv >= 0.9.9", logs.output[0]) | ||
| self.assertTrue(all(record.levelname == "DEBUG" for record in logs.records)) |
There was a problem hiding this comment.
[GENERAL] These two fallback tests (test_resolve_workspace_dir_logs_debug_for_old_uv and test_resolve_workspace_dir_falls_back_for_explicitly_unsupported_command) pass project_dir = os.path.join("path", "to"), a relative path. _log_workspace_fallback then does os.path.abspath(project_dir) and walks the real filesystem all the way to the root, reading every ancestor pyproject.toml. If any ancestor of the test process cwd contains a [tool.uv.workspace] table, the helper emits a WARNING record and these assertions fail:
self.assertIn("uv >= 0.9.9", logs.output[0])
self.assertTrue(all(record.levelname == "DEBUG" for record in logs.records))This repo's root pyproject.toml has no [tool.uv] section, so CI passes today, but the outcome depends on directories outside the checkout (e.g. a developer's ~/pyproject.toml or a parent workspace containing the clone).
test_workspace_hint_skips_non_table_values_and_continues_to_parent already guards against exactly this, with a comment saying so:
# Stop at the temporary root so unrelated manifests outside the test cannot warn.
real_dirname = os.path.dirname
with patch("os.path.dirname", side_effect=lambda path: root if path == root else real_dirname(path)):Applying the same containment here (or using a tempfile.TemporaryDirectory as project_dir with the traversal bounded) would make these two tests independent of the machine they run on.
| experimental_flags=self.experimental_flags, | ||
| ) | ||
| self.assertEqual(lock_path.read_bytes(), lock_contents) | ||
| self.assertEqual(lock_path.stat().st_mtime_ns, lock_mtime) |
There was a problem hiding this comment.
[GENERAL] test_workflow_reuses_existing_workspace_lock asserts the workspace lockfile is byte-identical and has an unchanged st_mtime_ns:
self.assertEqual(lock_path.read_bytes(), lock_contents)
self.assertEqual(lock_path.stat().st_mtime_ns, lock_mtime)Patching _build_from_pyproject prevents the builder from running uv lock, but _build_from_lock_file still runs uv export without --frozen/--locked, and export performs its own lock maintenance. The checked-in fixture pins version = 1 / revision = 3; whenever uv bumps the lock revision or changes serialization, export rewrites uv.lock in place and both assertions fail even though the behavior under test (reusing the workspace-root lock instead of regenerating one under app/) is still correct.
The behavior you actually care about is already covered by the _build_from_pyproject patch plus assertFalse(os.path.exists(os.path.join(source_dir, "uv.lock"))). Consider dropping the st_mtime_ns comparison (and ideally the byte comparison) so a uv upgrade doesn't turn this into a CI failure that requires regenerating the fixture.
… the workspace layout
…ns (< 0.9.9) that do not support workspaces
… is a UV workspace if a TOML parser is available
|
@roger-zhangg The bot review requested several additional fixes. The main changes are as follows:
As a result, the overall changes became quite extensive, so I’ve split them into commits that are neither too small nor too large, making them easier to review. |
2 participants
This PR fixes an issue where an application created as a member of a
uv workspace would fail to build if they depended on other workspace
members.
The following shows the problematic workspace structure and the error
message.
Even though
libandsam-appare in the same directory level in theworkspace, the workflow attempts to install
libundersam-app.In the workflow,
uv exportoutputs a list of dependency packages,which is then passed to
uv pip installfor installation. When doingso,
uv exportoutputs relative paths from the workspace root.Therefore,
uv pip installmust be run from the workspace root, notfrom the application directory.
Additionally, dependencies on other packages within the workspace are
exported as editable installations (e.g.,
-e ./lib) by default.When this is passed to
uv pip install, only the.pth(pathconfiguration) file for the package will be installed without the
package body. To prevent this, the
--no-editableoption needs to beused.
Closes #892.
Commands to reproduce the build failure
uv init --bare uv init --lib lib sam init --name sam-app --runtime python3.14 --architecture arm64 \ --dependency-manager pip --package-type Zip \ --app-template hello-world cd sam-app uv init uv add lib@../lib # remove requirements.txt and edit the app # edit template.yaml to configure `BuildMethod: python-uv` and `CodeUri: .` sam build --beta-featuresFixes #892