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

Allow selecting installed optional dependencies in Docker build #548

Merged
merged 2 commits into from
Dec 21, 2021

Conversation

juhoinkinen
Copy link
Member

Currently we provide a Dockerfile and an image in quay.io with dependencies for all the optional features.

If someone does not need all the features, but e.g. uses only Omikuji backend and would like build their own image (e.g. for smaller size), they could edit the Dockerfile dropping the unwanted (Python) dependencies from the pip install command. However, using --build-arg option of docker build provides a way to pass the user-defined list of dependencies to install that overrides the default list:

docker build -t annif-omikuji-voikko --build-arg optional_dependencies=omikuji,voikko .

A user wanting to have a Docker image to use with Xtransfomer could add pecos to the default list in build time.

@codecov
Copy link

codecov bot commented Dec 21, 2021

Codecov Report

Merging #548 (38adcf8) into master (5c6af91) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #548   +/-   ##
=======================================
  Coverage   99.49%   99.49%           
=======================================
  Files          80       80           
  Lines        5313     5313           
=======================================
  Hits         5286     5286           
  Misses         27       27           

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 5c6af91...38adcf8. Read the comment docs.

@osma
Copy link
Member

osma commented Dec 21, 2021

What about fasttext? It will be installed regardless of this setting, right?

@juhoinkinen
Copy link
Member Author

What about fasttext? It will be installed regardless of this setting, right?

Yes, that's true, I thought it would not be installed... Or at least not available right away. Hmm, I don't if its copying from builder stage can be controlled this way conveniently enough.

@osma
Copy link
Member

osma commented Dec 21, 2021

I don't think always including fasttext is a very big deal, this PR is still a big improvement.

That said, I think it could be possible to avoid installing fasttext by making the pip install command on line 9 conditional. Something like:

if [[ $optional_dependencies =~ "fasttext" ]]; then pip install --no-cache-dir fasttext==0.9.2; fi

(perhaps the apt-get install build-essential command could be moved inside the if clause as well, to avoid doing useless extra work in case fasttext isn't needed)

The ARG declaration needs to be moved earlier of course.

The COPY command would then copy an empty /usr/local/lib/python3.8 directory from the builder image.

@sonarcloud
Copy link

sonarcloud bot commented Dec 21, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Dockerfile Show resolved Hide resolved
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

LGTM

@juhoinkinen juhoinkinen merged commit 1710894 into master Dec 21, 2021
@juhoinkinen juhoinkinen deleted the docker-build-arg-for-optional-deps branch December 21, 2021 15:46
@juhoinkinen
Copy link
Member Author

I added a section of customizing Docker image to Wiki: https://github.com/NatLibFi/Annif/wiki/Usage-with-Docker#customizing-docker-image

@osma osma mentioned this pull request Feb 4, 2022
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.

2 participants