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

Clean up Docker Compose and add test #668

Merged
merged 13 commits into from
May 5, 2020

Conversation

woop
Copy link
Member

@woop woop commented May 1, 2020

What this PR does / why we need it:

  • Creates a GitHub Actions test for testing the current Docker Compose configuration. This won't test the latest commit's code (on PR or push). It will test based on what is configured on that commit. Currently master is set to use the latest docker image for the docker-compose setup, with the SDK using v0.4.7.
  • Modularizes the docker compose files into three separate files. One for the Core and dependencies, one for online, and one for the batch/historical store. Otherwise users will experience a failure on startup since jobs won't start if Feast Batch Serving registers itself without Feast Core having a GCP service account.
  • Uses a minimal notebook that is faster to download

Which issue(s) this PR fixes:

Fixes #652

Does this PR introduce a user-facing change?:

NONE

Problems with the current setup
A new release has to be made to backport the tutorials from master into 0.4. That would allow us to clean the tutorials up a bit to match the Docker Compose. The current tutorial has unnecessary steps like reinstalling the SDK.

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: woop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@woop woop changed the title Add docker compose test Clean up Docker Compose and add test May 1, 2020
@woop woop added the kind/bug label May 1, 2020
Copy link
Collaborator

@zhilingc zhilingc left a comment

Choose a reason for hiding this comment

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

Does this require any updates to the documentation?

@woop
Copy link
Member Author

woop commented May 1, 2020

Does this require any updates to the documentation?

Yes it does, but I will update that through gitbook since it is more convenient.

@mrzzy
Copy link
Collaborator

mrzzy commented May 3, 2020

Modularizes the docker compose files into three separate files. One for the Core and dependencies, one for online, and one for the batch/historical store.

I am in favor of a singular docker-compose file if possible in the future. Maybe we could ensure that all the dependencies is loaded before exposing its gRPC port? Then we can synchronize the order that Core/Dependencies and Serving has to started up in the future with wait-for-it automatically.

Otherwise users will experience a failure on startup since jobs won't start if Feast Batch Serving registers itself without Feast Core having a GCP service account.

Perhaps we should make this this failure explicit by making Batch Serving to refuse to run if Feast core does not have the required service account. Seems like otherwise this could be a silent error that can catch new users by surprise.

@woop
Copy link
Member Author

woop commented May 3, 2020

I am in favor of a singular docker-compose file if possible in the future.

So am I. Right now I am just trying to make the startup process explicit. It's still a one-liner so I dont see it being too painful.

Then we can synchronize the order that Core/Dependencies and Serving has to started up in the future with wait-for-it automatically.

Makes sense. Quite like the wait-for-it script!

Perhaps we should make this this failure explicit by making Batch Serving to refuse to run if Feast core does not have the required service account.

Yes, ideally that would be the case. The system shouldnt load if the configuration isn't set up correctly.

However, this would require a moderate amount of code changes because job management only detects the problem when it spins up new jobs after feature set registration. The fact that Feast Core needs the service account is the bigger problem. Which is why we want to move job management to Serving so that failure can be immediate and localized. I am not in favour of adding more complexity between these two services until we have done that.

@woop woop added the lgtm label May 5, 2020
@feast-ci-bot feast-ci-bot merged commit 4db87f3 into feast-dev:master May 5, 2020
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.

Docker Compose setup is broken on master
4 participants