-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add docker-compose file with Jupyer notebook #328
Add docker-compose file with Jupyer notebook #328
Conversation
Hi @khorshuheng. Thanks for your PR. I'm waiting for a gojek member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This is awesome, thanks @khorshuheng. Did you run any tests to confirm that this was working? |
77dbbbe
to
b92602c
Compare
The run output is included in the quick start jupyter notebook: https://github.com/gojek/feast/blob/b92602c2af2e5599e20e2a5f7bc72e657974b8e4/infra/docker-compose/jupyter/notebooks/feast-quickstart.ipynb I don't think i can write any unit test / integration test for this, but i can potentially include more scenarios in the quick start notebook to test different cases. What other scenarios do you think will be useful for the notebook example? We can also add additional command in the Makefile for starting / stopping the docker compose services, though i am not sure if that is needed for now. |
Hi @khorshuheng. Yea I dont think its necessary to automate the process. We just need to have a basic script where we can apply, ingest, and retrieve successfully. I just want a way to make sure that the docker compose file and configuration works. |
/ok-to-test |
e18d4d7
to
23584c0
Compare
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.
Seriously happy about this :)
I think it would be great to add a few lines in a README.md here just to explain how somebody can get up and running (most likely an end user, not a developer)
infra/docker/test-runner/conftest.py
Outdated
def pytest_addoption(parser): | ||
parser.addoption("--core_url", action="store", default="core:6565") | ||
parser.addoption("--serving_url", action="store", default="serving:6566") | ||
parser.addoption("--allow_dirty", action="store", default="True") |
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.
Is there a reason why you are setting allow-dirty to be true?
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.
My intention is to run the e2e test as follows:
docker-compose -f docker-compose.yml -f docker-compose.override.yml -f docker-compose.e2e.yml run e2e
That way, the e2e test run is entirely self contained. However, the above command will keep the services running even after the e2e test run finished. So for convenience i simply set allow dirty equals to true, otherwise we will need to restart the other services everytime. But i do agree that i should have probably set it to false, for consistency and accuracy of test.
That being say, this e2e test setup will be invalidated once we added test for batch retrieval, because my docker-compose setup has not included batch serving yet, only online serving.
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.
The only reason I added that flag was for safety, just in case somebody accidentally runs the test while connected to a production cluster. If you think that situation won't occur, then I guess its fine to make it true.
infra/docker/test-runner/Dockerfile
Outdated
ADD infra/docker/test-runner/conftest.py ./e2e/ | ||
ADD infra/docker/test-runner/run-e2e-test.sh ./ | ||
RUN chmod +x run-e2e-test.sh | ||
CMD ["./run-e2e-test.sh"] |
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.
Is this just running the normal e2e tests?
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.
Yes. This is also a way to test the docker compose setup.
infra/docker-compose/.env.sample
Outdated
@@ -0,0 +1,16 @@ | |||
COMPOSE_PROJECT_NAME=feast | |||
|
|||
FEAST_VERSION=0.3.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.
Should we perhaps default to latest
and just ensure that we always push the latest tagged docker image?
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.
Will do.
infra/docker-compose/.env.sample
Outdated
FEAST_SERVING_STORE_CONFIG_PATH=/etc/feast/feast-serving/store.yaml | ||
FEAST_SERVING_GCP_SERVICE_ACCOUNT_KEY=placeholder | ||
|
||
FEAST_JUPYTER_IMAGE=gcr.io/kf-feast/feast-jupyter |
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.
Not finding this. Did you end up pushing it, or should I help with that?
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.
I haven't pushed it yet. The current dockerfile always build from master branch, which is fine, but for production we should probably install via pip instead. For which versions should the Feast jupyter images be available? All versions after and including 0.3? (0.3.0, 0.3.2)
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.
Yes, ideally all versions.
Will add reference link to the Gitbook in the README.md |
23584c0
to
c2389c3
Compare
Add capability for batch serving using Big Query. Other changes:
|
/test test-golang-sdk |
1 similar comment
/test test-golang-sdk |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: khorshuheng, 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 |
7411765
to
c613115
Compare
/lgtm |
This addresses #272.
This PR added a new Jupyter notebook docker image preinstalled with Feast Python SDK. A quick start notebook has also been included for the corresponding docker compose service.
By default, the docker compose file will not rebuild the image. Instead, it will pull images based on the environmental variable defined under infra/docker-compose/.env. Users can either use the default setting, in which the official Feast images are used, or substitute one or more images with their own local image.
Additional settings, such as google credential key, Springboot configurations can be configured via the .env file as well.
Developers who wish to rebuild everything from scratch may run docker-compose -f docker-compose.yml -f docker-compose.dev.yml up -d.
Pending tasks before this can be merged: