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

Fix Docker integration docs for intermediate layers #7166

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

NiklasRosenstein
Copy link

Summary

The Docker integration docs were missing to mount the uv.lock and pyproject.toml which are required also for the second layer that runs uv sync --locked.

It also failed to mention that if your pyproject.toml references a readme file, that it must be mounted as well.

@NiklasRosenstein
Copy link
Author

NiklasRosenstein commented Sep 7, 2024

I would also suggest that we add --link-mode=copy to the command to prevent this warning:

#11 0.340 warning: Failed to hardlink files; falling back to full copy. This may lead to degraded performance.
#11 0.340          If the cache and target directories are on different filesystems, hardlinking may not be supported.
#11 0.340          If this is intentional, set `export UV_LINK_MODE=copy` or use `--link-mode=copy` to suppress this warning.

Uv detects that the cache sits on a different filesystem (yay!), but it's probably a good idea to make this explicit in the Dockerfile.

It's also mandatory to use this option, even if you don't use a cache mount, if you plan on copying only your project directory from another stage (e.g. when combining multiple stages), you likely won't think about copying (and merging!) the UV cache directory. Admittedly, that is a bit of an edge case.

Adding --compile-bytecode might also be a good idea!

edit: I've made these changes in ea61ed1, let me know what you think.

Comment on lines +350 to +351
--mount=type=bind,source=uv.lock,target=uv.lock \
--mount=type=bind,source=pyproject.toml,target=pyproject.toml \
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't these be added by the ADD . /app invocation?

Copy link
Author

@NiklasRosenstein NiklasRosenstein Sep 7, 2024

Choose a reason for hiding this comment

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

Oh my, I blame tunnel vision. 🤦

In my case I added only the src directory because ADD . /app adds a lot of things that I don't want to add. Thinking about it, with --link-mode=copy, I could probably even --mount the source directory itself as well? Arguably, a very minor optimization as you don't usually have many hundreds of megabytes of source code.

Copy link
Member

@zanieb zanieb Sep 7, 2024

Choose a reason for hiding this comment

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

The source directory needs to stick around because we don't support non-editable installs of workspace members during syncs yet (#5792). Maybe it'd be best to address your case in the documentation by adding a case to the optimization examples for copying the src directory instead of the full project directory?

@@ -340,18 +340,31 @@ WORKDIR /app
RUN --mount=type=cache,target=/root/.cache/uv \
--mount=type=bind,source=uv.lock,target=uv.lock \
--mount=type=bind,source=pyproject.toml,target=pyproject.toml \
uv sync --frozen --no-install-project
uv sync --frozen --no-install-project --link-mode=copy --compile-bytecode
Copy link
Member

Choose a reason for hiding this comment

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

I think I have a preference for setting an environment variable for this as in astral-sh/uv-docker-example#16

I think we cover bytecode compilation in a separate example though https://docs.astral.sh/uv/guides/integration/docker/#compiling-bytecode

Copy link

Choose a reason for hiding this comment

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

I agree that bytecode compilation shouldn't be included in this example, as it is already covered afterwards

Copy link
Author

@NiklasRosenstein NiklasRosenstein Sep 7, 2024

Choose a reason for hiding this comment

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

Fair point, although I feel obliged to point out that it also talks about caching later but uses a cache mount in this example already. :)

Copy link
Member

Choose a reason for hiding this comment

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

I think I missed that when someone added the bind mounts to the example in #6921 — we should probably drop that from here too for clarity.

@zanieb
Copy link
Member

zanieb commented Sep 7, 2024

Thanks for contributing! I have a couple questions :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants