Skip to content

feat: Support custom predictor Docker image builds on non-x86 architectures #2115

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Mar 6, 2025
Merged

feat: Support custom predictor Docker image builds on non-x86 architectures #2115

merged 11 commits into from
Mar 6, 2025

Conversation

TeoZosa
Copy link
Contributor

@TeoZosa TeoZosa commented Apr 19, 2023 •
edited
Loading

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)
  • Get the necessary approvals
  • Once the last commit on the PR has been approved, add the "ready to pull" label to the Pull Request

Note: do not merge your PR from GitHub. Adding the "ready to pull" label is the final step in the review process.
After approvals, the changes in your PR will be committed to the main branch and this PR will be closed.

Fixes 🦕:

What

Allows users the ability to specify for which platform their custom predictor's Docker image should be built.

  • Defaults to linux/amd64
    • Currently only machine types with 64-bit x86 processors are supported for prediction (see here) and custom training (see here)
  • Because of the above, Docker image builds now work out of the box for users on compatible non-x86 systems (for instance, Apple Silicon)

Warning

I would imagine that, in an ideal world, we should be testing this on compatible non-x86 systems (e.g., arm64 and aarch64)?

Copilot-generated summary This pull request introduces support for specifying a target platform when building Docker images and models. The changes involve updates to the `build_image` and `build_cpr_model` functions, as well as modifications to the corresponding test cases to accommodate the new `platform` parameter.

Key changes include:

Docker Image Building Enhancements:

  • Added an optional platform parameter to the build_image function in google/cloud/aiplatform/docker_utils/build.py. This allows users to specify the target platform for the Docker image build. [1] [2] [3]

Model Building Enhancements:

  • Added an optional platform parameter to the build_cpr_model function in google/cloud/aiplatform/prediction/local_model.py. This parameter allows users to specify the target platform for building the local model Docker image. [1] [2] [3] [4]

Testing Enhancements:

  • Updated the test_build_cpr_model_upload_and_deploy test case in tests/system/aiplatform/test_prediction_cpr.py to include a platform parameter, allowing the test to run for different platforms. [1] [2]

Why

For library compatibility on compatible non-x86 systems (for instance, Apple Silicon).
@google-cla
Copy link

google-cla bot commented Apr 19, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: vertex-ai Issues related to the googleapis/python-aiplatform API. labels Apr 19, 2023
@TeoZosa TeoZosa changed the title [WIP] Support Docker image builds on non-x86 processors [WIP] Support custom predictor Docker image builds on non-x86 processors Apr 19, 2023
@TeoZosa TeoZosa changed the title [WIP] Support custom predictor Docker image builds on non-x86 processors Support custom predictor Docker image builds on non-x86 processors Apr 19, 2023
@TeoZosa TeoZosa changed the title Support custom predictor Docker image builds on non-x86 processors feat: Support custom predictor Docker image builds on non-x86 processors Apr 19, 2023
@TeoZosa TeoZosa changed the title feat: Support custom predictor Docker image builds on non-x86 processors feat: Support custom predictor Docker image builds on non-x86 architectures Apr 19, 2023
@TeoZosa TeoZosa marked this pull request as ready for review April 19, 2023 08:25
@TeoZosa TeoZosa force-pushed the feat/build-local-model-docker-images-on-64-bit-arm branch from 37abc45 to 9c37f8e Compare April 20, 2023 00:54
@TeoZosa TeoZosa force-pushed the feat/build-local-model-docker-images-on-64-bit-arm branch from 9c37f8e to 4d5d703 Compare April 28, 2023 08:44
@TeoZosa
Copy link
Contributor Author

TeoZosa commented Apr 29, 2023

Not sure how to tag to trigger unit tests or who to ping.

@sararob, I hope this isn't a bother, and please no rush. I'm Teo and I'm a first-time contributor to this project. Maybe I missed something in the different contributing docs on how to move a PR forward to the unit test/review stage. Since you've had some recent activity on main would you have any pointers? Thank you in advance!
@sasha-gitg sasha-gitg added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels May 9, 2023
abcdefgs0324
abcdefgs0324 reviewed May 23, 2023
View reviewed changes
google/cloud/aiplatform/docker_utils/build.py Outdated
pip_command: str = "pip",
python_command: str = "python",
no_cache: bool = True,
platform: str = "linux/amd64",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would prefer this flag to be set by users, rather providing a universal default.

Could we update the default to be None?
@TeoZosa TeoZosa force-pushed the feat/build-local-model-docker-images-on-64-bit-arm branch from 4d5d703 to 811c0e1 Compare June 11, 2023 00:08
@rragundez
Copy link
Contributor

rragundez commented Jul 19, 2024 •
edited
Loading

Hi, can someone help taking a look at this? I would imagine you would like to support all the users using new Apple machines...
Me and my team would benefit directly from this feature.

@abcdefgs0324 or is there a better/correct way to do this?
@TeoZosa
Copy link
Contributor Author

TeoZosa commented Aug 13, 2024

@rragundez if it helps I can also push the change suggested by @abcdefgs0324 (assuming it still applies?)
TeoZosa added a commit to TeoZosa/python-aiplatform that referenced this pull request Aug 14, 2024
@TeoZosa
Change platform default arg to None
f7217ff 
To enforce the flag is set by users as opposed to providing a universal default. - See: googleapis#2115 (comment)
@TeoZosa TeoZosa requested a review from abcdefgs0324 August 14, 2024 09:44
TeoZosa added a commit to TeoZosa/python-aiplatform that referenced this pull request Aug 15, 2024
@TeoZosa
Change platform default arg to None
a407b77 
To enforce the flag is set by users as opposed to providing a universal default. - See: googleapis#2115 (comment)
@TeoZosa TeoZosa force-pushed the feat/build-local-model-docker-images-on-64-bit-arm branch from f7217ff to 6482c3c Compare August 15, 2024 02:26
@TeoZosa TeoZosa force-pushed the feat/build-local-model-docker-images-on-64-bit-arm branch from 26b6049 to 1e6bdea Compare August 23, 2024 01:41
@frankcaoyun
Copy link

Hi @TeoZosa @abcdefgs0324, would you be kindly able to help to move this PR forward? Our local deployment is blocked by this issue for Mac users and we have some urgent use cases that want to fulfill. Really appreciate your help!
@TeoZosa
Copy link
Contributor Author

TeoZosa commented Feb 3, 2025

@frankcaoyun Would love to have this merged. I wonder if @abcdefgs0324 is still assigned to this. I'll look for another active maintainer to ping so we can move this forward.
@frankcaoyun
Copy link

@frankcaoyun Would love to have this merged. I wonder if @abcdefgs0324 is still assigned to this. I'll look for another active maintainer to ping so we can move this forward.

Thanks for the prompt reply @TeoZosa !
@TeoZosa
Copy link
Contributor Author

TeoZosa commented Feb 6, 2025

@frankcaoyun we reached out to GCP solutions engineers who are reaching out to the Vertex AI team. Hopefully we can get movement on this in the next week.
@frankcaoyun
Copy link

frankcaoyun commented Feb 6, 2025 via email

abcdefgs0324
abcdefgs0324 reviewed Mar 4, 2025
View reviewed changes
Copy link
Contributor

@abcdefgs0324 abcdefgs0324 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed this for a long time. Do you mind adding unit tests?
google/cloud/aiplatform/prediction/local_model.py Outdated
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#leverage-build-cache
for more details.
platform (str):
Required. The target platform for the Docker image build. See
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional.
google/cloud/aiplatform/docker_utils/build.py Outdated
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#leverage-build-cache
for more details.
platform (str):
Required. The target platform for the Docker image build. See
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional.
TeoZosa added a commit to TeoZosa/python-aiplatform that referenced this pull request Mar 4, 2025
@TeoZosa
Change platform default arg to None
244a359 
To enforce the flag is set by users as opposed to providing a universal default. - See: googleapis#2115 (comment)
TeoZosa added a commit to TeoZosa/python-aiplatform that referenced this pull request Mar 4, 2025
@TeoZosa
Fix docstring for platform param
2420e85 
Resolves: - googleapis#2115 (comment) - googleapis#2115 (comment)
@TeoZosa TeoZosa force-pushed the feat/build-local-model-docker-images-on-64-bit-arm branch from f65b061 to b4f6095 Compare March 4, 2025 05:20
abcdefgs0324
abcdefgs0324 approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor

@abcdefgs0324 abcdefgs0324 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
@sasha-gitg sasha-gitg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 4, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 4, 2025
@abcdefgs0324
Copy link
Contributor

Could you fix the unit tests and maybe add a test case to https://github.com/googleapis/python-aiplatform/blob/main/tests/unit/aiplatform/test_prediction.py?
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Mar 5, 2025
@TeoZosa TeoZosa force-pushed the feat/build-local-model-docker-images-on-64-bit-arm branch from aeb0bde to d8a3d2f Compare March 5, 2025 13:19
@TeoZosa
Copy link
Contributor Author

TeoZosa commented Mar 5, 2025 •
edited
Loading

Could you fix the unit tests and maybe add a test case to https://github.com/googleapis/python-aiplatform/blob/main/tests/unit/aiplatform/test_prediction.py?

@abcdefgs0324 Done. Thanks for the patience. It's been a while, and I forgot that we can run tests outside of CI 🙇 .

Let me know if the changes in b4f6095...d8a3d2f make sense.
@abcdefgs0324 abcdefgs0324 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 5, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 5, 2025
@abcdefgs0324 abcdefgs0324 added the ready to pull Ready to be merged into the codebase. label Mar 5, 2025
@abcdefgs0324 abcdefgs0324 added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed ready to pull Ready to be merged into the codebase. labels Mar 5, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 5, 2025
@abcdefgs0324 abcdefgs0324 added the ready to pull Ready to be merged into the codebase. label Mar 5, 2025
@abcdefgs0324 abcdefgs0324 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2025
@abcdefgs0324 abcdefgs0324 merged commit 87dd5c0 into googleapis:main Mar 6, 2025
17 checks passed
@release-please release-please bot mentioned this pull request Mar 6, 2025
Merged
copybara-service bot pushed a commit that referenced this pull request Mar 10, 2025
@abcdefgs0324 @copybara-github
Copybara import of the project:
5fb8cf1 
-- Enforce Linux `x86_64` Docker image builds. Under the assumption that only machine types with x86 processors are supported for prediction and custom training. Extract Docker build platform arg to method parameter. COPYBARA_INTEGRATE_REVIEW=#2115 PiperOrigin-RevId: 735416705
jhpierce pushed a commit to jhpierce/python-aiplatform that referenced this pull request Mar 11, 2025
@TeoZosa @abcdefgs0324 @jhpierce
feat: Support custom predictor Docker image builds on non-x86 archite…
b49a0d4 
…ctures (googleapis#2115) * Enforce Linux `x86_64` Docker image builds Under the assumption that only machine types with x86 processors are supported for prediction and custom training. * Extract Docker build platform arg to method parameter While currently only x86 processors are supported for prediction and custom training, this will allow users to control this behavior should that ever change in the future. Additionally, it allows users to, e.g., override the `TARGETOS` component of the `TARGETPLATFORM`. * Change platform default arg to `None` To enforce the flag is set by users as opposed to providing a universal default. - See: googleapis#2115 (comment) * Make platform configurable from `LocalModel.build_cpr_model()` To enable the flag to be set by users (e.g., to build images on non-x86 architectures). * Fix docstring for `platform` param Resolves: - googleapis#2115 (comment) - googleapis#2115 (comment) * Test platform parameter in `test_build_cpr_model_upload_and_deploy()` Resolves: - googleapis#2115 (review) * Fix tests Resolves (partially): - googleapis#2115 (comment) * Test specifying platform in local model builds Resolves (partially): - googleapis#2115 (comment) * Test other platform strings in local model builds --------- Co-authored-by: Chun-Hsiang Wang <chunhsiang@google.com>
jhpierce pushed a commit to jhpierce/python-aiplatform that referenced this pull request Mar 11, 2025
@abcdefgs0324 @jhpierce
Copybara import of the project:
85d4e8e 
-- Enforce Linux `x86_64` Docker image builds. Under the assumption that only machine types with x86 processors are supported for prediction and custom training. Extract Docker build platform arg to method parameter. COPYBARA_INTEGRATE_REVIEW=googleapis#2115 PiperOrigin-RevId: 735416705
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: vertex-ai Issues related to the googleapis/python-aiplatform API. ready to pull Ready to be merged into the codebase. size: m Pull request size is medium.

6 participants

@TeoZosa @rragundez @frankcaoyun @abcdefgs0324 @sasha-gitg @yoshi-kokoro