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

Feature/add docker compose #1250

Merged
merged 9 commits into from
Oct 26, 2021
Merged

Feature/add docker compose #1250

merged 9 commits into from
Oct 26, 2021

Conversation

davidbuniat
Copy link
Member

🚀 🚀 Pull Request

Checklist:

  • My code follows the style guidelines of this project and the Contributing document
  • I have commented my code, particularly in hard-to-understand areas
  • I have kept the coverage-rate up
  • I have performed a self-review of my own code and resolved any problems
  • I have checked to ensure there aren't any other open Pull Requests for the same change
  • I have described and made corresponding changes to the relevant documentation
  • New and existing unit tests pass locally with my changes

Changes

Modified the dockerfile for complete local environment and added a docker-compose file for easy parameters

@CLAassistant
Copy link

CLAassistant commented Oct 11, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #1250 (a303a80) into main (bfcced8) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1250      +/-   ##
==========================================
+ Coverage   91.24%   91.26%   +0.02%     
==========================================
  Files         139      139              
  Lines        9768     9768              
==========================================
+ Hits         8913     8915       +2     
+ Misses        855      853       -2     
Impacted Files Coverage Δ
hub/core/lock.py 84.44% <0.00%> (+2.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfcced8...a303a80. Read the comment docs.

bin/docker-compose.yaml Show resolved Hide resolved
bin/docker-compose.yaml Show resolved Hide resolved
@davidbuniat davidbuniat removed the request for review from AbhinavTuli October 12, 2021 19:28
@gautamkrishnar
Copy link
Contributor

Hi @davidbuniat since we are gonna use this compose for development workflow I recommend the following approach:

  1. Revert the changes done to bin/Dockerfile
  2. Create a new bin/Dockerfile.dev with the following contents:
FROM python:3.8-slim

RUN apt-get -y update && \
    apt-get -y install git wget build-essential python-setuptools python3-dev libjpeg-dev libpng-dev zlib1g-dev && \
    apt install build-essential

RUN mkdir /app

ADD ./hub/requirements /app/hub/requirements
WORKDIR /app

ENV PYTHONPATH="/app/Hub:$PYTHONPATH"
RUN pip install -r hub/requirements/requirements.txt && \
    pip install -r hub/requirements/common.txt && \
    pip install -r hub/requirements/tests.txt && \
    pip install -r hub/requirements/plugins.txt
  1. Update the bin/docker-compose.yaml to the following values:
version: '2'
x-creds: &global-vars
  environment:
    - AWS_ACCESS_KEY_ID
    - AWS_SECRET_ACCESS_KEY
    - AWS_DEFAULT_REGION
    - GOOGLE_APPLICATION_CREDENTIALS=/root/.config/gcloud/gcs.json
    - ACTIVELOOP_HUB_PASSWORD

services:
  local:
    build:
      context: ../.
      dockerfile: ./bin/Dockerfile.dev
    volumes:
      - ../:/app
    command: >
      bash -c "python setup.py install && python3 -m pytest -x --local"

  complete:
    build:
      context: ../.
      dockerfile: ./bin/Dockerfile.dev
    <<: *global-vars
    volumes:
      - ~/.config/gcloud/gcs.json:/root/.config/gcloud/gcs.json
      - ../:/app
    command: >
      bash -c "python setup.py install && python3 -m pytest -x --local --s3 --gcs --hub-cloud --ignore-glob=buH/*"

@davidbuniat
Copy link
Member Author

I like this approach more, but I don't see why python setup.py install needs to be run every time before starting the docker. One time installing through pip3 install -e . ensures that the python package is loaded to the most recent one (even after changes have been done).

@gautamkrishnar
Copy link
Contributor

I like this approach more, but I don't see why python setup.py install needs to be run every time before starting the docker. One time installing through pip3 install -e . ensures that the python package is loaded to the most recent one (even after changes have been done).

It will make sure that the installed package is always the latest one. You can also use the 'pip3 install -e .' command. Let's say a developer changed the source code on his local environment after cloning the repo. He had already done building the image earlier after cloning the repo. Having it in the start script will always take the latest version of the package. Tbh it is just a hack so that developer don't have to pass '--build' argument to docker compose all the time.

@davidbuniat
Copy link
Member Author

@gautamkrishnar exactly that case is covered with install being in dockerfile, because you are also mapping the volume. Any change after docker is built will be reflected in the python package. I do my development like that. Sometimes I often rerun the docker and there is no need to install the package every time.

@davidbuniat
Copy link
Member Author

@mccrearyd those creds do not get stored inside docker, they are environment variables that are passed when you run docker-compose up. If you have stored the environment variables inside Dockerfile, then they would be stored.

Copy link
Contributor

@gautamkrishnar gautamkrishnar left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

Copy link
Contributor

@verbose-void verbose-void left a comment

Choose a reason for hiding this comment

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

address comments, ping for quick rereview

Comment on lines 14 to 19
RUN pip install -r hub/requirements/requirements.txt && \
pip install -r hub/requirements/common.txt && \
pip install -r hub/requirements/tests.txt && \
pip install -r hub/requirements/plugins.txt

RUN pip3 install -e .
Copy link
Contributor

Choose a reason for hiding this comment

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

using pip3 and pip could break things if we change FROM container

also, if you run pip install -e . first then you only need to install tests/plugins.

finally, we need to remove requirements.txt (it's useless). we should only have "common.txt"

WORKDIR /app

ENV PYTHONPATH="/app/Hub:$PYTHONPATH"
RUN ls hub/
Copy link
Contributor

Choose a reason for hiding this comment

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

why ls?

bin/docker-compose.yaml Show resolved Hide resolved
@davidbuniat
Copy link
Member Author

@mccrearyd in addition to plugins.txt we still need install tests.txt if we want to run pytest inside docker

@verbose-void verbose-void merged commit 4a6bfe7 into main Oct 26, 2021
@verbose-void verbose-void deleted the feature/add_docker_compose branch October 26, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants