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

Shell out to docker buildx build to build images #1402

Merged
merged 22 commits into from
Feb 13, 2025

Conversation

yuvipanda
Copy link
Collaborator

@yuvipanda yuvipanda commented Jan 29, 2025

pydocker uses the HTTP API (equivalent to docker build) which has been dead for ages. docker buildx (the interface to BuildKit) is what we should be using, but alas pydocker does not currently support it and I suspect it never will (docker/docker-py#2230).

This PR simply shells out, as I think that's the way.

I'm going to list some intentional choices I've made here.

  1. Intentionally replace the build functionality of the existing DockerEngine than make a new one. The old way is deprecated and we should not support it.
  2. Continue using dockerpy for all other non-build operations. It works fine and I don't see a need to change it right now.
  3. Simply extract the tarball we construct elsewhere. This is kinda double work, but we can treat this as a possible optimization pathway in the future.
  4. Currently ignore container_limits (because buildx doesn't support it, it should be set at the level of the builder instead)

Questions to answer

  • What do we do about container_limits here?
  • We probably need to deprecate / throw errors on some existing traitlets as they won't really have any effect here. What traitlets are those?
  • We should treat this as a breaking change and make a release after, right?

Traitlets to deprecate

  • Repo2Docker.build_memory_limit - this must be instead set up by the builder

Fixes #875

pydocker uses the HTTP API (equivalent to `docker build`) which
has been dead for ages. `docker buildx` (the interface to BuildKit)
is what we *should* be using, but alas pydocker does not currently
support it and I suspect it never will (docker/docker-py#2230).

This PR simply shells out, as I think that's the way.

Fixes jupyterhub#875
@yuvipanda
Copy link
Collaborator Author

This isn't complete, putting it up here to see how many tests fail.

@yuvipanda yuvipanda requested review from manics and minrk January 29, 2025 04:41
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Nice! This will also unblock using COPY --chmod as in #1395

Re: extra_build_args, presumably we do want to keep an equivalent passthrough for extra args to pass to buildx build on the command-line (a list this time)?

repo2docker/docker.py Show resolved Hide resolved
platform=platform,
**kwargs,
)
if not shutil.which("docker"):
Copy link
Member

Choose a reason for hiding this comment

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

Looks like rm and forcerm don't have equivalents. Just noting that this is intentional. Are there likely to be significant changes in the cache behavior with buildkit we need to pay attention to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we'll have to see how it goes. But buildkit does give us a lot more knobs here - https://docs.docker.com/build/cache/backends/.

@yuvipanda
Copy link
Collaborator Author

@minrk I added extra_buildx_build_args in what feels like the right place for it. Will need to figure out a test.

yuvipanda added a commit to yuvipanda/mybinder.org-deploy that referenced this pull request Jan 30, 2025
Trying to fix https://jupyter.zulipchat.com/#narrow/channel/469744-jupyterhub/topic/try.20mybinder.20jupyterleab.20demo.20broken,
which is breaking because mamba gets OOM killed. We have memory limits
set on dind as a whole, and memory_limit for builds (in its current,
per-build form) is going away as part of jupyterhub/repo2docker#1402
as well. So let's remove it and see.
repo2docker/docker.py Outdated Show resolved Hide resolved
@minrk
Copy link
Member

minrk commented Jan 30, 2025

Will need to figure out a test.

iidfile looks like it might be a good arg to test with, since it just creates a file to check. Or --check appears to skip the build entirely, so should be quick.

@minrk
Copy link
Member

minrk commented Jan 30, 2025

Here's a small test for extra buildx args with --check that goes really quick since it doesn't actually build the image:

python3 -m repo2docker --no-run '--DockerEngine.extra_buildx_build_args=["--check"]' .

which outputs (among other things):

Check complete

yuvipanda added a commit to yuvipanda/mybinder.org-deploy that referenced this pull request Feb 8, 2025
Follow-up to jupyterhub#3204,
which failed because it removed the limit, which caused it to be
lower than the request. All the reasons given in that PR still apply.

Here I bump up the limit enough until the super popular
https://github.com/jupyterlab/jupyterlab-demo is able to build.
It's not a functionally super complex environment.yml, and even with
simplifications in jupyterlab/jupyterlab-demo#147,
it fails to build without the memory increase limit. As mentioned in
the other PR, jupyterhub/repo2docker#1402 will
get rid of this kind of limit anyway, so it's ok to raise this.
yuvipanda and others added 2 commits February 11, 2025 12:17
Co-authored-by: Min RK <benjaminrk@gmail.com>
@@ -69,6 +74,14 @@ class DockerEngine(ContainerEngine):
config=True,
)

extra_buildx_build_args = List(
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth pushing this up a level to ContainerEngine under a more generic name? It shouldn't be a breaking change since it'll be ignored by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@manics if we change the container engine, the parameters passed to this necessarily need to change, as these aren't portable across engines. so you may want to pass different params to podman vs here. so I think it's useful to just leave them over here

@yuvipanda
Copy link
Collaborator Author

I was wrong about Repo2Docker.extra_build_args - as this is purely about pydocker's build method that we no longer use - that's actually BUILD ARGS haha

@yuvipanda
Copy link
Collaborator Author

Also, instead of having a separate builder traitlet, we can pass a different dockerx builder if needed with the generic extra args.

I have made this intentionally fail if --build-memory-limit is passed, which I think is the right call here. It clearly requires action from the admin, as their memory usage will no longer be limited.

@yuvipanda
Copy link
Collaborator Author

I actually don't think we need to do any newline mangling here. I think this is good to go!

@manics @minrk lmk if you think this needs anything else :) When we make a CHANGELOG, i can add a BREAKING entry

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Nice! This is ok to go for me

repo2docker/app.py Outdated Show resolved Hide resolved
@manics
Copy link
Member

manics commented Feb 12, 2025

I was wrong about Repo2Docker.extra_build_args - as this is purely about pydocker's build method that we no longer use - that's actually BUILD ARGS haha

If Repo2Docker.extra_build_args is already passed as build_args do we still need extra_buildx_build_args?

@yuvipanda
Copy link
Collaborator Author

@manics those are for build args that are declared via ARG in dockerfile, rather than additional arguments to the build process! so yeah i do think we need that.

Co-authored-by: Simon Li <orpheus+devel@gmail.com>
repo2docker/__main__.py Outdated Show resolved Hide resolved
Co-authored-by: Simon Li <orpheus+devel@gmail.com>
@yuvipanda
Copy link
Collaborator Author

Exciting. I'll merge and test / deploy ideally tomorrow.

@yuvipanda yuvipanda merged commit 13cbc9e into jupyterhub:main Feb 13, 2025
20 checks passed
yuvipanda added a commit to yuvipanda/binderhub that referenced this pull request Feb 13, 2025
docker buildx wants to write to ~/.docker/buildx, but since
we mount the secret *entirely* (to ~/.docker/buildx), it can not.
I think mounting *just* the subpath should work.

Follow-up to jupyterhub/repo2docker#1402
yuvipanda added a commit to yuvipanda/binderhub that referenced this pull request Feb 13, 2025
docker buildx wants to write to ~/.docker/buildx, but since
we mount the secret *entirely* (to ~/.docker/buildx), it can not.
I think mounting *just* the subpath should work.

Follow-up to jupyterhub/repo2docker#1402
yuvipanda added a commit to yuvipanda/scikit-learn that referenced this pull request Feb 14, 2025
.dockerenv is only supported in the old docker based builder, which has been deprecated for a few years now. It isn't supported by buildkit / docker buildx, which repo2docker *just* moved to (jupyterhub/repo2docker#1402). Using buikdkit gives us faster and more reliable builds.

This converts the test for checking if the script is being run by repo2docker to check for an env var that repo2docker sets instead.
yuvipanda added a commit to yuvipanda/mybinder.org-deploy that referenced this pull request Feb 14, 2025
Brings in:

repo2docker (among others):
- jupyterhub/repo2docker#1402
- jupyterhub/repo2docker#1413

binderhub:
- jupyterhub/binderhub#1929
- jupyterhub/binderhub#1930

Note that this *could* cause some issues for the very few repos
that depend on internal details of the non buildx old builder.
For example, see scikit-learn/scikit-learn#30835
yuvipanda added a commit to yuvipanda/mybinder.org-deploy that referenced this pull request Feb 14, 2025
Brings in:

repo2docker (among others):
- jupyterhub/repo2docker#1402
- jupyterhub/repo2docker#1413

binderhub:
- jupyterhub/binderhub#1929
- jupyterhub/binderhub#1930

Note that this *could* cause some issues for the very few repos
that depend on internal details of the non buildx old builder.
For example, see scikit-learn/scikit-learn#30835

We also remove memory limits, as those are not supported right now
via the docker buildx build. We will need to add support for custom
buildkit builders in binderhub, and then we can use those.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to using Buildkit
3 participants