-
Notifications
You must be signed in to change notification settings - Fork 102
feat: add multi-platform ARM64 support for dependency packaging #361
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: main
Are you sure you want to change the base?
Conversation
a006067 to
ec0b7ea
Compare
ec0b7ea to
8d6009d
Compare
8d6009d to
722d07f
Compare
| platforms = ["aarch64-manylinux2014", "aarch64-manylinux_2_17", "aarch64-manylinux_2_28"] | ||
|
|
||
| for i, platform in enumerate(platforms): | ||
| cmd = self._build_uv_command(requirements_file, target_dir, python_version, platform) |
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.
so we running the compile for each platform seperately. Can we not do it together all at once like we do in pip?
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.
Unfortunately uv does not give option to have multiple python platform args with fallback
astral-sh/uv#3333
| log.info("✓ Dependencies installed with uv") | ||
| except subprocess.CalledProcessError as e: | ||
| raise RuntimeError(f"Failed to install dependencies with uv: {e.stderr}") from e | ||
| if cross_compile: |
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 do we need this flag?
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 flag is in the method by default, in case we want to add local compilation in the future.
Right now this flag always returns true (utility method)
| def _build_uv_command(self, requirements: Path, target: Path, py_version: str, cross: bool) -> List[str]: | ||
| try: | ||
| subprocess.run(cmd, check=True, capture_output=True, text=True) # nosec B603 - using uv command | ||
| if i == 0: |
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.
Did not understand this part, why are we checking like this?
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 needs to be fixed. Will address
722d07f to
588125b
Compare
|
Do we install multiple binaries with this approach, or just one of the available ones? |
|
So uv pip install resolves the dependencies, then actually installs them. The resolution fails fast. So this installs a single binary for the dependencies for the first python platform that is matched |
- Support multiple manylinux platforms: aarch64-manylinux2014, aarch64-manylinux_2_17, aarch64-manylinux_2_28 - Try platforms in order of preference for better wheel compatibility - Fallback to next platform if current one fails to find compatible wheels
588125b to
b02a271
Compare
Adds support for multiple ARM64 manylinux platforms to improve wheel compatibility when packaging dependencies for AgentCore Runtime.
Changes
Benefits