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

Build Feast Jupyter image and clean up examples #803

Merged

Conversation

woop
Copy link
Member

@woop woop commented Jun 18, 2020

What this PR does / why we need it:

  • Builds a base image for Jupyter (gcr.io/kf-feast/feast-jupyter:latest) so that it's faster for new users to start the Jupyter container for Docker Compose. Feast dependencies are still installed upon startup, but most of them should now already be available.
  • Cleaned up the Basic example a little bit.
  • Fixed entity_df bug in Telco example and added more details

Fixes: #742

@algattik
Copy link

Funny, I've done exactly the same thing today on our fork :)

Here's my dockerfile (maximizing layer reuse). It seems to me that there is no need anymore for the startup script to download and install stuff, and downloading release versions in the dev cycle makes config management harder?

FROM jupyter/minimal-notebook:619e9cc2fc07

USER root

WORKDIR /feast

# Init empty .git settings (required by SDK setup.py)
RUN git init .

COPY sdk/python sdk/python
COPY Makefile Makefile
COPY protos protos
COPY tests tests
COPY README.md README.md

# Install Python dependencies
RUN make compile-protos-python

# Install CI requirements (only needed for running tests)
RUN pip install -r sdk/python/requirements-ci.txt

# Install Feast SDK
RUN pip install -e sdk/python -U

# Switch back to original user and workdir
USER $NB_UID
WORKDIR $HOME

CMD start-notebook.sh --NotebookApp.token=''

@woop
Copy link
Member Author

woop commented Jun 18, 2020

@algattik neat!

I might reuse some of yours. I will still be cloning the repository since the image I am building is not meant for development, it's meant to have the latest stable dependencies preinstalled, and I guess we can have the actual SDK also preinstalled.

I don't like mapping in the examples directly from the repo because master is normally not consistent with the stable branch, and if we pin versions then its very hard to backport tutorials or make small tweaks.

@woop
Copy link
Member Author

woop commented Jun 19, 2020

@algattik Made some changes to find a better middle ground. Hope it looks better :)

I didn't attribute the Dockerfile to you, but normally people don't care 🎖️

@woop
Copy link
Member Author

woop commented Jun 19, 2020

/test test-end-to-end-batch

@woop woop changed the title Docker Compose housekeeping Docker Compose and Example housekeeping Jun 19, 2020
command: ["/etc/startup.sh"]
command: >
bash -c "
git clone https://github.com/feast-dev/feast.git || true
Copy link
Collaborator

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 can't we bake in the Feast repo into the docker image?

Copy link
Member Author

Choose a reason for hiding this comment

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

The latest image will only be built on a release, which means that if we want to roll out the latest tutorials then we have to cut a release. Often I find myself updating docs/tutorials between releases, which is why I have pinned this clone to the branch instead. In theory we could build the latest image on every commit, but that image will go stale very quickly on the user's side (they have to repull the whole time).

@woop woop changed the title Docker Compose and Example housekeeping Clean up Docker Compose and examples Jun 19, 2020
@woop woop changed the title Clean up Docker Compose and examples Build Feast Jupyter image and clean up examples Jun 19, 2020
@woop
Copy link
Member Author

woop commented Jun 19, 2020

/test test-end-to-end-batch

WORKDIR /feast

# .git (required by SDK setup.py)
COPY .git .git
Copy link

@algattik algattik Jun 19, 2020

Choose a reason for hiding this comment

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

.git and README.md are only needed by " pip install -e sdk/python" IIRC, so if you move them down you can reuse layer with requirements installed. (.git folder changes with every commit)

Copy link
Member Author

Choose a reason for hiding this comment

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

Corrected, thanks.

@algattik
Copy link

@algattik Made some changes to find a better middle ground. Hope it looks better :)

I didn't attribute the Dockerfile to you, but normally people don't care 🎖️

My ego shall survive :)

@woop woop added the lgtm label Jun 19, 2020
@zhilingc
Copy link
Collaborator

/approve

@zhilingc zhilingc self-requested a review June 19, 2020 08:35
@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: woop, zhilingc

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

@feast-ci-bot feast-ci-bot merged commit 64e5bc4 into feast-dev:master Jun 19, 2020
@feast-ci-bot
Copy link
Collaborator

@woop: Updated the config configmap in namespace default at cluster default using the following files:

  • key config.yaml using file .prow/config.yaml

In response to this:

What this PR does / why we need it:

  • Builds a base image for Jupyter (gcr.io/kf-feast/feast-jupyter:latest) so that it's faster for new users to start the Jupyter container for Docker Compose. Feast dependencies are still installed upon startup, but most of them should now already be available.
  • Cleaned up the Basic example a little bit.
  • Fixed entity_df bug in Telco example and added more details

Fixes: #742

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.

woop added a commit that referenced this pull request Jun 19, 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.

Fix Telecom Customer Churn Prediction Example Notebook
5 participants