Conversation
asottile
left a comment
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
| # 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
| except Exception: | ||
| pass |
|
Thanks for your attention and fast response @asottile!
My test commands were executing 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
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
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. |
|
I don't think what you're attempting is possible. any number of options could appear there. |
|
Pre-commit's public docs state that I've updated this branch to add more test cases and simply skip the precaching attempt if any Docker flags other than (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.) |
|
then simply run docker pull in github actions there's no way we can accurately simulate the same commandline parsing as docker |
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 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. |
only first run will incur that cost -- afterwards it's free |
2 participants
Currently
language: docker_imagewill 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_imageto 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.