-
Notifications
You must be signed in to change notification settings - Fork 153
feat: Python UV workflow #756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
ec932ce
to
84f5c13
Compare
class MissingUvError(LambdaBuilderError): | ||
"""Exception raised when UV executable is not found.""" | ||
|
||
MESSAGE = "uv executable not found in your environment. Please install uv: https://docs.astral.sh/uv/getting-started/installation/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also print the {uv_path} in the message, to mention the specific location where the uv executable was not found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on how it's used here, I don't think we'd get an exact location back.
artifacts_dir_path: str, | ||
scratch_dir_path: str, | ||
manifest_path: str, | ||
architecture: str = X86_64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we hardcode the architecture here?
handler = self._get_manifest_handler(manifest_name) | ||
|
||
# Execute the handler | ||
handler(manifest_path, artifacts_dir_path, scratch_dir_path, python_version, architecture, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this method actually building the dependencies? For reference here is how python pip handles building the dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sriram-mv can correct me if I'm wrong, but it seems like handler will be the function that is returned by _get_manifest_handler
, which returns either _handle_pyproject_build
or _build_from_requirements
. Those go on to call one of the functions in UvRunner
. I'm also writing this comment to confirm my own understanding.
except Exception as e: | ||
raise UvBuildError(reason=f"Failed to build from pyproject.toml: {str(e)}") | ||
|
||
def _export_pyproject_to_requirements( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we converting pyproject.toml
to requirements.txt
, when it is recommended t o use pyproject.toml
to define dependencies for UV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I also don't quite follow. It seems like if there's a lock file we use uv sync
, and if there's not we convert to requirements and then use uv pip install
. My understanding is that we can use uv sync
even without the lock file, so I feel like the distinction between lock file and no lock file might not be useful. I could easily be overlooking something entirely, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
• uv sync does NOT support --python-platform
• uv pip install DOES support --python-platform for cross-platform builds
this is the reason I believe.
MESSAGE = "Lock file operation failed: {reason}" | ||
|
||
|
||
class ManifestNotFoundError(LambdaBuilderError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where have we used this exception?
MESSAGE = "Failed to install dependencies using uv: {reason}" | ||
|
||
|
||
class UvResolutionError(LambdaBuilderError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where have we used this exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note to this, after changes are made for PR feedback, we need to make sure that DESIGN.md is updated accordingly as it is referenced there as well.
@patch("aws_lambda_builders.workflows.python_uv.packager.OSUtils") | ||
def test_subprocess_uv_init_missing_uv(self, mock_osutils_class): | ||
mock_osutils = Mock() | ||
mock_osutils.which.return_value = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just keep the path here /usr/bin/
@sriram-mv do you think this could be further extended to also support poetry? Basically if an interface is included here (something like PythonPyprojectDepedenciesBuilder), then there can be a |
@sriram-mv @Vandita2020 will this PR ever be merged? UV has gained a lot of attention in the past year, and just using |
If there's not any movement on this PR in a couple weeks and I have some free time, I will try to update this PR with the feedback after taking a look. |
@reedham-aws I'll pick it up maybe later next week. Is there any other feedback that I can address in one go apart from what's mentioned? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look too hard at the tests, and overall the code looks good. I have a couple code quality suggestions and a question about the way the code chooses to do uv pip install
versus uv sync
.
# Determine target directory | ||
target_artifact_dir = self.artifacts_dir | ||
if self.dependencies_dir: | ||
target_artifact_dir = self.dependencies_dir | ||
|
||
# Build dependencies | ||
package_builder.build_dependencies( | ||
artifacts_dir_path=target_artifact_dir, | ||
scratch_dir_path=self.scratch_dir, | ||
manifest_path=self.manifest_path, | ||
architecture=self.architecture, | ||
config=self.config, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put this section inside of a try/except
like
try: | |
target_artifact_dir = self.artifacts_dir | |
# if dependencies folder is provided, download the dependencies into dependencies folder | |
if self.dependencies_dir: | |
target_artifact_dir = self.dependencies_dir | |
package_builder.build_dependencies( | |
artifacts_dir_path=target_artifact_dir, | |
scratch_dir_path=self.scratch_dir, | |
requirements_path=self.manifest_path, | |
) | |
except PackagerError as ex: | |
raise ActionFailedError(str(ex)) |
class MissingUvError(LambdaBuilderError): | ||
"""Exception raised when UV executable is not found.""" | ||
|
||
MESSAGE = "uv executable not found in your environment. Please install uv: https://docs.astral.sh/uv/getting-started/installation/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on how it's used here, I don't think we'd get an exact location back.
MESSAGE = "Failed to install dependencies using uv: {reason}" | ||
|
||
|
||
class UvResolutionError(LambdaBuilderError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note to this, after changes are made for PR feedback, we need to make sure that DESIGN.md is updated accordingly as it is referenced there as well.
except Exception as e: | ||
raise UvBuildError(reason=f"Failed to build from requirements: {str(e)}") | ||
|
||
def _extract_python_version(self, runtime: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _extract_python_version(self, runtime: str) -> str: | |
def _extract_python_version(self, runtime: Optional[str]) -> str: |
because when this is used the runtime is optional.
raise ActionFailedError(f"UV build failed: {str(ex)}") | ||
|
||
|
||
class CopyDependenciesAction(BaseAction): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason to have this when a different CopyDependenciesAction exists?
if not config.cache_dir: | ||
uv_cache_dir = os.path.join(scratch_dir_path, "uv-cache") | ||
# Use exist_ok equivalent for osutils | ||
if not os.path.exists(uv_cache_dir): | ||
self._osutils.makedirs(uv_cache_dir) | ||
config.cache_dir = uv_cache_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This same cache_dir setup is done in both of the main UvRunner
functions, is probably redundant either here or there.
handler = self._get_manifest_handler(manifest_name) | ||
|
||
# Execute the handler | ||
handler(manifest_path, artifacts_dir_path, scratch_dir_path, python_version, architecture, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sriram-mv can correct me if I'm wrong, but it seems like handler will be the function that is returned by _get_manifest_handler
, which returns either _handle_pyproject_build
or _build_from_requirements
. Those go on to call one of the functions in UvRunner
. I'm also writing this comment to confirm my own understanding.
"""Get the appropriate handler function for a manifest file.""" | ||
# Exact match handlers for ACTUAL manifests | ||
exact_handlers = { | ||
"pyproject.toml": self._handle_pyproject_build, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't really have a problem with this structure, just curious if this is left as a dict so we can have further handlers in the future? Not understanding why this was done.
except Exception as e: | ||
raise UvBuildError(reason=f"Failed to build from pyproject.toml: {str(e)}") | ||
|
||
def _export_pyproject_to_requirements( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I also don't quite follow. It seems like if there's a lock file we use uv sync
, and if there's not we convert to requirements and then use uv pip install
. My understanding is that we can use uv sync
even without the lock file, so I feel like the distinction between lock file and no lock file might not be useful. I could easily be overlooking something entirely, though.
) | ||
] | ||
|
||
def _get_additional_binaries(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed for UV?
Core Features: * Intelligent handling of pyproject.toml with automatic uv.lock detection * Multi-format support: pyproject.toml, requirements.txt, and requirements-*.txt variants * Lock file optimization: Automatic uv.lock usage for reproducible builds when available * Platform targeting: Lambda-compatible builds for x86_64 and ARM64 architectures * Python version control: Precise Python version targeting via UV's --python flags UV Functionality: * `uv sync --python X.Y` for lock-based builds (pyproject.toml + uv.lock) * `uv lock && uv export` workflow for pyproject.toml without lock files * `uv pip install --python-version X.Y --python-platform` for requirements.txt * Proper virtual environment handling and site-packages extraction Documentation: * Complete DESIGN.md with architecture overview and usage patterns * Accurate API documentation reflecting constructor + method call pattern * Smart dispatch logic clearly explained with examples * Updated to match actual implementation (no outdated references) Compatibility: * Compatible with existing Lambda Builders interface * Supports standard build parameters (source_dir, artifacts_dir, scratch_dir) * Runtime parameter properly extracted and used for Python version targeting * Architecture parameter mapped to UV platform specifications
Issue link: #743
Core Features:
UV Functionality:
uv sync --python X.Y
for lock-based builds (pyproject.toml + uv.lock)uv lock && uv export
workflow for pyproject.toml without lock filesuv pip install --python-version X.Y --python-platform
for requirements.txtDocumentation:
Compatibility:
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.