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

feat: ✨ Official docker images for docTR #1322

Merged
merged 26 commits into from
Sep 28, 2023

Conversation

odulcy-mindee
Copy link
Collaborator

@odulcy-mindee odulcy-mindee commented Sep 18, 2023

TODO

  • TF Dockerfile
    Still heavy TBH:
REPOSITORY                                 TAG                                              IMAGE ID       CREATED             SIZE
test_doctr_gpu                             latest                                           936c9e203093   17 minutes ago      8.22GB
  • PT Dockerfile
  • CI job to publish docTR image

@odulcy-mindee odulcy-mindee changed the title feat: ✨ tf Dockerfile feat: ✨ Official docker images for docTR Sep 18, 2023
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #1322 (9f7c649) into main (3271460) will increase coverage by 0.01%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1322      +/-   ##
==========================================
+ Coverage   95.78%   95.79%   +0.01%     
==========================================
  Files         154      154              
  Lines        6902     6902              
==========================================
+ Hits         6611     6612       +1     
+ Misses        291      290       -1     
Flag Coverage Δ
unittests 95.79% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
doctr/models/classification/predictor/pytorch.py 95.45% <100.00%> (ø)
doctr/models/detection/predictor/pytorch.py 95.23% <100.00%> (ø)
doctr/models/kie_predictor/pytorch.py 100.00% <100.00%> (ø)
doctr/models/predictor/pytorch.py 100.00% <100.00%> (ø)
doctr/models/recognition/parseq/pytorch.py 97.16% <100.00%> (ø)
doctr/models/recognition/parseq/tensorflow.py 97.33% <100.00%> (ø)
doctr/models/recognition/predictor/pytorch.py 91.66% <100.00%> (ø)
doctr/models/recognition/vitstr/pytorch.py 100.00% <100.00%> (ø)
doctr/models/recognition/vitstr/tensorflow.py 97.61% <100.00%> (ø)

... and 1 file with indirect coverage changes

@odulcy-mindee odulcy-mindee marked this pull request as ready for review September 25, 2023 14:28
@odulcy-mindee odulcy-mindee added type: enhancement Improvement topic: docker Docker-related os: linux Involving Linux users type: new feature New feature labels Sep 25, 2023
@felixT2K
Copy link
Contributor

felixT2K commented Sep 26, 2023

Further todo's:

  • Add a documentation page

  • Add a badge in the readme which links to the published images

  • Rethink the CI job (push to hub on publish - as discussed in Slack)

@odulcy-mindee Anything missing ? ^^

Dockerfile Outdated Show resolved Hide resolved
@odulcy-mindee
Copy link
Collaborator Author

Further todo's:

That's good ! 👍

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@odulcy-mindee
Copy link
Collaborator Author

@felixdittrich92 FYI:

test_doctr                                 latest                                           523853251752   About a minute ago   4.01GB
test_doctr_gpu                             latest                                           a7866d727078   9 minutes ago        9.02GB

Copy link
Contributor

@felixT2K felixT2K left a comment

Choose a reason for hiding this comment

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

@odulcy-mindee Really good job that's a great and useful feature 👍🏼

I added only a few comments and questions

.github/workflows/public_docker_images.yml Outdated Show resolved Hide resolved
.github/workflows/public_docker_images.yml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved

- name: Push Docker image
# Push only if the CI is not triggered by "PR on main"
if: github.ref == 'refs/heads/main' && github.event_name != 'pull_request'
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not 100% sure that this works on publish (release)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@felixT2K Mmmh, actually it works for tags but it's not done on publish: [released] as I'm afraid the CI job metadata from docker does not handle this event. I'll double check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@felixT2K I did a test on my fork. When a draft release is created, a new tag is not created. When the release is published, the tag is created, so it triggers the on: push: tags event 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

nice :)

pull_request:
branches: main
schedule:
- cron: '0 2 29 * *' # At 02:00 on day-of-month 29
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@felixT2K I changed the cron so it should be triggered tomorrow morning. We'll see if it works or not

Copy link
Contributor

Choose a reason for hiding this comment

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

alright 👍🏼

@felixdittrich92 felixdittrich92 added this to the 0.7.1 milestone Sep 28, 2023
@felixdittrich92 felixdittrich92 linked an issue Sep 28, 2023 that may be closed by this pull request
@felixT2K
Copy link
Contributor

great job 🤗 thanks

@odulcy-mindee
Copy link
Collaborator Author

🚀

@odulcy-mindee odulcy-mindee merged commit 69f6705 into mindee:main Sep 28, 2023
69 of 70 checks passed
@odulcy-mindee odulcy-mindee deleted the official_docker_images branch September 28, 2023 15:52
@ffalkenberg
Copy link
Contributor

where are the prebuilt docker images hosted?

@ffalkenberg
Copy link
Contributor

Having reviewed your Dockerfile, it seems that it's configured to operate as root by default. This is a significant security risk and is likely unacceptable for most production environments.

@odulcy-mindee
Copy link
Collaborator Author

Hello @ffalkenberg,

where are the prebuilt docker images hosted?

You can find the docker images here. You can also have a look at this section in the README.md

it seems that it's configured to operate as root by default.

Yes, the container runs as root, as it did before. I understand your concern. For that, I suggest you take a look at what Jupyter has done, they have such an interesting approach to handling that, but it's really not trivial. This deserves a dedicated Pull Request to handle these user/permission issues.

@odulcy-mindee
Copy link
Collaborator Author

@ffalkenberg

sh: 14: python: not found

It's python3

$ /opt/python

It's a folder.

Moreover, this works:

$ docker run -it --rm -u 1000 ghcr.io/mindee/doctr:torch-py3.9.18-gpu-2023-09 bash
I have no name!@451b261aabdb:/$ python3
Python 3.10.13 (main, Sep 27 2023, 16:35:15) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> 
I have no name!@451b261aabdb:/$ 

So it's not a permission issue.

For the demo app, you can still craft your own Docker image, based on the previous modification:

FROM ghcr.io/mindee/doctr:torch-py3.9.18-gpu-2023-09

ENV DOCTR_CACHE_DIR=/app/.cache 
WORKDIR /app

COPY . .

RUN chmod -R a+w /app

You can also add a dedicated user in the container but it's out of scope of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os: linux Involving Linux users topic: docker Docker-related type: enhancement Improvement type: new feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation for running on docker
4 participants