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 dockerfile #93

Merged
merged 2 commits into from
Jun 19, 2020
Merged

fix dockerfile #93

merged 2 commits into from
Jun 19, 2020

Conversation

cehbrecht
Copy link
Member

Overview

This PR fixes the dockerfile in the template.

Changes:

  • Fixed the conda version spec for pywps.

Related Issue / Discussion

PR #92.

Additional Information

@cehbrecht cehbrecht added the bug Something isn't working label Jun 18, 2020
@cehbrecht cehbrecht added this to the 1.0.0 milestone Jun 18, 2020
@tlvu
Copy link

tlvu commented Jun 18, 2020

Copied from #92 (comment)

I would not use the existing environment.yml ... it has too many dependencies which are not used in the docker container ... makes the image larger ...

You surprised me with this statement, I always thought environment.yml is a one to one match with requirements.txt which should be the strict minimum for production runtime. All "dev" extra dependencies are captured in requirements_dev.txt. But you are right, there are tests dependencies in environment.yml.

Since this the cookiecutter which should be "clean", I'd suggest we fix the polluted environment.yml and use environment.yml in the Dockerfile instead.

Copy link

@tlvu tlvu left a comment

Choose a reason for hiding this comment

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

Since this the cookiecutter which should be "clean", I'd suggest we fix the polluted environment.yml and use environment.yml in the Dockerfile instead.

@cehbrecht
Copy link
Member Author

Since this the cookiecutter which should be "clean", I'd suggest we fix the polluted environment.yml and use environment.yml in the Dockerfile instead.

I'm not sure ... we are lazy :) ... and we like to add packages to the conda env like pytest. Should we use a special conda env for docker (docker/environment.yml)? I suppose there is no inheritance for conda envs to avoid duplications.

@huard thoughts?

@huard
Copy link
Collaborator

huard commented Jun 18, 2020

We have two uses cases:

  • installing a development environment
  • deploying a production server

Ideally we'd use the same environment in both, as this would reduce the likelihood of errors creeping up in prod.

Is the additional space required by the "dev" stuff significant ? If not, then I would not mind installing additional packages into the docker image.
If it is, we might need to think about this a bit more.

@tlvu
Copy link

tlvu commented Jun 18, 2020

we like to add packages to the conda env like pytest

I thought that's what the make develop target is meant for

.PHONY: develop
develop:
@echo "Installing development requirements for tests and docs ..."
@-bash -c 'pip install -e ".[dev]"'

I also have a question: do we prefer Conda or Pip/setup.py? We seems to be using both interchangeably randomly right now. I would thought that we would prefer Conda and fallback to Pip/setup.py only if no Conda package exist. Conda have the advantage that it sandboxes also system package while Pip will need system packages and if these system packages are too old, we are stuck with the old distro version.

@huard
Copy link
Collaborator

huard commented Jun 18, 2020

Good point, solved then.
Agreed, conda is a safer options for packages with tricky dependencies.

@cehbrecht
Copy link
Member Author

I have build a docker image with and without test conda packages:

  • pywps only: 1.31 GB
  • with test packages: 1.75 GB

@cehbrecht
Copy link
Member Author

What is the solution now?

  • use the conda env also in docker image (we need xarray, dask, netcdf, ... in our birds)
  • skip the packages for tests in conda environment.yml? Maybe keep pytest ... but not ipython, notebook, nbsphinx, ...
  • dev packages will be installed with for example make develop. This will be a pip installation in a conda environment ... I think it should be safe.

@cehbrecht
Copy link
Member Author

I have updated this PR to proposed solution .... we can still revert/tune it.

Docker image size with pytest: 1.46GB

@cehbrecht
Copy link
Member Author

@tlvu can we use cruft to have a part in the conda environment.yml which is maintained by cruft and additional packages are untouched (to avoid merge conflicts)? I think this is done for requirements_dev.txt.

@huard
Copy link
Collaborator

huard commented Jun 19, 2020

Related to this discussion is the idea of running the tests suite after the docker build, to ensure that docker images are healthy (#84). To me, this suggests that the docker environment should contain the minimal set of packages to run those tests, or least a subset of the tests.

@cehbrecht
Copy link
Member Author

Related to this discussion is the idea of running the tests suite after the docker build, to ensure that docker images are healthy (#84). To me, this suggests that the docker environment should contain the minimal set of packages to run those tests, or least a subset of the tests.

does this mean we should add more packages to the conda environment.yml?

@huard
Copy link
Collaborator

huard commented Jun 19, 2020

I'm just saying this is a discussion I think we should have, weigh the pros and cons. We could take a few minutes today or next week to discuss this face to face if you want. What's your take on this ?

@tlvu
Copy link

tlvu commented Jun 19, 2020

What is the solution now?

* use the conda env also in docker image (we need xarray, dask, netcdf, ... in our birds)

* skip the packages for tests in conda `environment.yml`? Maybe keep pytest ... but not ipython, notebook, nbsphinx, ...

* dev packages will be installed with for example `make develop`. This will be a pip installation in a conda environment ... I think it should be safe.

Agree with all you said, except hopefully not needed to keep pytest or any dev related package in the production image.

can we use cruft to have a part in the conda environment.yml which is maintained by cruft and additional packages are untouched (to avoid merge conflicts)? I think this is done for requirements_dev.txt.

Agreed I think we could do the same trick as requirements_dev.txt.

Related to this discussion is the idea of running the tests suite after the docker build, to ensure that docker images are healthy (#84). To me, this suggests that the docker environment should contain the minimal set of packages to run those tests, or least a subset of the tests.

does this mean we should add more packages to the conda environment.yml?

Not sure how I will attack that issue yet but if I can I will live install all the extra test packages at test time so the production image is clear of all unnecessary test packages and to keep the image smaller.

I'm just saying this is a discussion I think we should have, weigh the pros and cons. We could take a few minutes today or next week to discuss this face to face if you want. What's your take on this ?

Should we open a separate issue listing the desired goals and current problems so we can get more feedback from other bird developers?

For now @cehbrecht you can merge this PR. We can open other PR later for more work. Thanks.

@cehbrecht cehbrecht merged commit 5351c2f into master Jun 19, 2020
@cehbrecht cehbrecht deleted the fix-docker-build branch June 19, 2020 17:40
Zeitsperre pushed a commit that referenced this pull request Jan 5, 2021
* fix conda version spec

* use conda environment.yml in docker build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants