Skip to content
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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 6 commits into from

Conversation

TeoZosa
Copy link

@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)?

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
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
@@ -418,6 +418,7 @@ def build_image(
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
Enforce Linux x86_64 Docker image builds
7b432be 
Under the assumption that only machine types with x86 processors are
supported for prediction and custom training.
@TeoZosa
Extract Docker build platform arg to method parameter
811c0e1 
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`.
@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

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
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
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
Make platform configurable from LocalModel.build_cpr_model()
6482c3c 
To enable the flag to be set by users (e.g., to build images on non-x86
architectures).
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abcdefgs0324 abcdefgs0324 Awaiting requested review from abcdefgs0324

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
api: vertex-ai Issues related to the googleapis/python-aiplatform API. size: s Pull request size is small.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TeoZosa @rragundez @abcdefgs0324 @sasha-gitg