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.

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

fix: Bad feature_servers/multicloud Dockerfile #4138

Closed

Conversation

bushwhackr
Copy link
Contributor

@bushwhackr bushwhackr commented Apr 23, 2024

What this PR does / why we need it:

Multicloud dockerfile is not consistent with the rest of the infra/feature_servers in that it pulls pypi feast package. Whilst others uses the repository code to build feast.

This PR seeks to standardise it so that we are not reliant on the pypi feast package.

NOTE: I am not a user of multicloud feast and do not understand its use case. The Dockerfile I created is based on my observation on what the incumbent was doing.

Which I understood as the following:

  1. Install feast into docker image
  2. Install runtime dependencies like libarrow-dev

I would need clarification on what is the purpose of this Docker image.

Which issue(s) this PR fixes:

#4135, Related: #3953

Fixes

@bushwhackr bushwhackr changed the title Fixing building multicloud feature_servers for feast fix: building feature_servers/multicloud Dockerfile Apr 23, 2024
@bushwhackr bushwhackr changed the title fix: building feature_servers/multicloud Dockerfile fix: feature_servers/multicloud Dockerfile Apr 23, 2024
@bushwhackr bushwhackr changed the title fix: feature_servers/multicloud Dockerfile fix: Bad feature_servers/multicloud Dockerfile Apr 23, 2024
@bushwhackr bushwhackr force-pushed the chester/multicloud-fix branch from 820ef16 to 0e22da4 Compare April 23, 2024 16:13
@bushwhackr
Copy link
Contributor Author

bushwhackr commented Apr 23, 2024

$ export VERSION=0.37.1
$ export REGISTRY=docker.io
$ make build-feature-server-docker

[+] Building 95.1s (14/14) FINISHED                                                                                                                                                                                                                                                                          docker:default
 => [internal] load build definition from Dockerfile                                                                                                                                                                                                                                                                   0.0s
 => => transferring dockerfile: 1.44kB                                                                                                                                                                                                                                                                                 0.0s
 => [internal] load metadata for docker.io/library/python:3.9-slim                                                                                                                                                                                                                                                     0.9s
    ...
    ...
 => exporting to image                                                                                                                                                                                                                                                                                                 3.6s
 => => exporting layers                                                                                                                                                                                                                                                                                                3.6s
 => => writing image sha256:7cd5076b1023823d01b98297b06544f2ab52964aa6c690ba6b2e8492a841decf                                                                                                                                                                                                                           0.0s
 => => naming to docker.io/library/feature-server:0.37.1
 
$ docker run -it docker.io/library/feature-server:0.37.1 pip show feast | grep Version
Version: 0.37.1.dev16+g0e22da42.d20240423

@bushwhackr
Copy link
Contributor Author

cc @jeremyary

@jeremyary
Copy link
Collaborator

Thanks! We'll take a look. As far as use case goes (as far as I understand), it's a stretch to imagine an end user utilizing every cloud provider we enable, but this checks the box for ensuring whatever variant of subsets a user plugs in are good to go together.

@bushwhackr
Copy link
Contributor Author

any blockers?

@jeremyary
Copy link
Collaborator

@bushwhackr apologies for taking a bit of time. We've had a backlog to work through after resolving our release process issues, so I'm slowly catching us up on PR's. If you wouldn't mind TAL at currrent conflicts, I'll work to bring in your change for our next release. Thanks again for your time spent here & contributions!

@franciscojavierarceo
Copy link
Member

i believe this is used during our deployment to verify the build

@shuchu
Copy link
Collaborator

shuchu commented May 29, 2024

I believe the original Dockerfile (with pypi installation) still useful while the users want to test official Feast releases.

@@ -395,7 +395,7 @@ build-feature-server-java-docker:
build-feature-server-dev:
docker buildx build --build-arg VERSION=dev \
-t feastdev/feature-server:dev \
-f sdk/python/feast/infra/feature_servers/multicloud/Dockerfile.dev --load .

Choose a reason for hiding this comment

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

was this used at all for deploying any images? Do you want to ask achal or any of the old maintainers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only difference in the previous Dockerfile vs Dockerfile.dev was the COPY . . instruction.

The old Dockerfile installs only the pypi available feast package, whereas the old Dockerfile.dev installs the pypi + copies the current codebase into the image.

With the changes I made to the Dockerfile, it does not rely on pypi for the feast package and we would only use the current codebase to install feast. Essentially the same as what old Dockerfile.dev was doing.

@shuchu
Copy link
Collaborator

shuchu commented May 29, 2024

I believe the original Dockerfile (with pypi installation) still useful while the users want to test official Feast releases.

ah, I know the problem. In the document, Development Guide part. The last step of Environment setup is:
docker build -t docker-whale -f ./sdk/python/feast/infra/feature_servers/multicloud/Dockerfile .

As a result, it will call the pypi version....

Signed-off-by: Chester Ong <chester.ong.ch@gmail.com>
Signed-off-by: Chester Ong <chester.ong.ch@gmail.com>
@bushwhackr bushwhackr force-pushed the chester/multicloud-fix branch from 0e22da4 to 015da38 Compare May 29, 2024 09:57
Signed-off-by: Chester Ong <chester.ong.ch@gmail.com>
@tokoko tokoko closed this Aug 22, 2024
@bushwhackr bushwhackr deleted the chester/multicloud-fix branch September 3, 2024 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants