Optimize Docker image pull - #3573
Conversation
asottile
left a comment
There was a problem hiding this comment.
I don't think this actually works. your output indicates you profiled against a pulled and a non-pulled image
the pull only happens once in the daemon so this doesn't actually do anything
Sorry, something went wrong.
| # To prevent duplicate simultaneous image pull attempts in `run_xargs`, we | ||
| # try to precache the Docker image by pulling it here first | ||
| try: | ||
| image_name = cmd[2 if cmd[0] == '--entrypoint' else 0] |
There was a problem hiding this comment.
this won't work reliably at all
Sorry, something went wrong.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
? this is never acceptable
Sorry, something went wrong.
|
Thanks for your attention and fast response @asottile!
My test commands were executing docker rmi ubuntu:22.04 before pre-commit to ensure that the image is non-pulled. I believe that the "Pulling from library/ubuntu" message appears only in the before run's output because in the after run, the pull occurs outside of lang_base.run_xargs rather than inside of it.
The very reproducible timing differences suggest to me that the speed improvement is real and quite large. Here are several more alternating before/after runs with docker rmi in between: $ git checkout main &> /dev/null $ docker rmi ubuntu:22.04 &> /dev/null; time pre-commit run test --all-files --verbose &> /dev/null pre-commit run test --all-files --verbose &> /dev/null 1.29s user 1.31s system 25% cpu 10.000 total $ git checkout optimize-docker-image-pull &> /dev/null $ docker rmi ubuntu:22.04 &> /dev/null; time pre-commit run test --all-files --verbose &> /dev/null pre-commit run test --all-files --verbose &> /dev/null 0.79s user 0.52s system 27% cpu 4.704 total $ git checkout main &> /dev/null $ docker rmi ubuntu:22.04 &> /dev/null; time pre-commit run test --all-files --verbose &> /dev/null pre-commit run test --all-files --verbose &> /dev/null 1.25s user 1.31s system 18% cpu 13.661 total $ git checkout optimize-docker-image-pull &> /dev/null $ docker rmi ubuntu:22.04 &> /dev/null; time pre-commit run test --all-files --verbose &> /dev/null pre-commit run test --all-files --verbose &> /dev/null 0.79s user 0.51s system 28% cpu 4.565 total $ git checkout main &> /dev/null $ docker rmi ubuntu:22.04 &> /dev/null; time pre-commit run test --all-files --verbose &> /dev/null pre-commit run test --all-files --verbose &> /dev/null 1.22s user 1.27s system 16% cpu 15.536 total $ git checkout optimize-docker-image-pull &> /dev/null $ docker rmi ubuntu:22.04 &> /dev/null; time pre-commit run test --all-files --verbose &> /dev/null pre-commit run test --all-files --verbose &> /dev/null 0.80s user 0.51s system 29% cpu 4.423 total
Fair enough. In this PR, I'm just trying to greatly improve the most common cases while leaving the less common cases the same as before. I didn't want to let the perfect be the enemy of the good. I'm happy to try modifying this PR to meet repo standards. In any case, this feels to me like a problem worth solving, whether in this PR or another one. |
Sorry, something went wrong.
|
I don't think what you're attempting is possible. any number of options could appear there. |
Sorry, something went wrong.
|
Pre-commit's public docs state that entry is just a Docker image tag with possibly an entrypoint override (as opposed to a full-blown docker run arg list), but fair enough: maybe there are users who are relying on undocumented behavior. I've updated this branch to add more test cases and simply skip the precaching attempt if any Docker flags other than --entrypoint are found. This optimizes all documented forms of entry while leaving the presumably much rarer undocumented usages untouched. (More background on Duolingo's use case: we're running pre-commit on PRs as a required status check via GitHub Actions, which doesn't cache pulled Docker images, so every second counts.) |
Sorry, something went wrong.
|
then simply run docker pull in github actions there's no way we can accurately simulate the same commandline parsing as docker |
Sorry, something went wrong.
That's our current workaround, but it doesn't help the normal use case of devs running pre-commit locally.
pre-commit.com doesn't promise full parsing parity, just the entrypoint args I've handled here. If maintaining exactly the same pre-commit behavior in all cases is worth forgoing this PR's improvement of the documented cases, it could be worth updating the docs to advertise that entry is actually a docker run arg list so that users can confidently take advantage of that fact. Anyway, you're now aware of the redundant download issue and I think I've offered all the help I can. Thanks again for your time here and for creating pre-commit - we've been using it heavily for nearly a decade. |
Sorry, something went wrong.
only first run will incur that cost -- afterwards it's free |
Sorry, something went wrong.
Currently language: docker_image will have each partition try to pull the same Docker image individually, which is unnecessarily slow because the image really only needs to be downloaded once.
This PR modifies language: docker_image to precache the image before running the entry command.
I tested in this repo with this hook:
Before this PR it takes 15 seconds:
After this PR it takes 4 seconds:
Note the before run's twelve occurrences of "Pulling from library/ubuntu" and the after run's 3.5x speed improvement.