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

build: Reduce intermediate layers #7408

Merged
merged 1 commit into from
Jul 8, 2024
Merged

build: Reduce intermediate layers #7408

merged 1 commit into from
Jul 8, 2024

Conversation

krishung5
Copy link
Contributor

@krishung5 krishung5 commented Jul 3, 2024

What does the PR do?

Reduce intermediate layers in order to reduce TRT-LLM container size.

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • Added succinct git squash message before merging ref.
  • All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Related PRs:

Where should the reviewer start?

Test plan:

TRT-LLM backend build + tests should pass.

  • CI Pipeline ID: 16294063

Caveats:

Background

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: #xxx

@krishung5 krishung5 changed the title Reduce intermediate layers build: Reduce intermediate layers Jul 3, 2024
@krishung5 krishung5 marked this pull request as ready for review July 3, 2024 17:44
@nv-kmcgill53
Copy link
Contributor

What is the improvement with this change?

@krishung5
Copy link
Contributor Author

It seems like every Dockerfile command, e.g. FROM, , RUN, etc. creates an image layer of its own. The layers are then used to assemble the final image. With more accumulated layers, the image will be increased in size.

rm -fr ${TRT_ROOT}/bin ${TRT_ROOT}/targets/${ARCH}-linux-gnu/bin ${TRT_ROOT}/data && \
rm -fr ${TRT_ROOT}/doc ${TRT_ROOT}/onnx_graphsurgeon ${TRT_ROOT}/python && \
rm -fr ${TRT_ROOT}/samples ${TRT_ROOT}/targets/${ARCH}-linux-gnu/samples && \
python3 -m pip install --upgrade pip && \
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather keep pip to have clean docker file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean just run pip install instead of python3 -m pip install?

RUN pip3 install setuptools==69.5.1 grpcio-tools==1.64.0
RUN ldconfig && \
ARCH="$(uname -i)" && \
rm -fr ${TRT_ROOT}/bin ${TRT_ROOT}/targets/${ARCH}-linux-gnu/bin ${TRT_ROOT}/data && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Double slash \\ in the and is an escape symbol was to give developers more clean and readable Dockerfile output. Not sure if it make sense to remove it.

@mc-nv
Copy link
Contributor

mc-nv commented Jul 5, 2024

It seems like every Dockerfile command, e.g. FROM, , RUN, etc. creates an image layer of its own. The layers are then used to assemble the final image. With more accumulated layers, the image will be increased in size.

Idea behind image layers is to keep layer history transparent.
There why it's better to keep them separate unless it vital.

@GuanLuo
Copy link
Contributor

GuanLuo commented Jul 5, 2024

Is there before / after comparison in terms of image size?

@krishung5
Copy link
Contributor Author

@GuanLuo The container size from the latest pipeline is now only 24.7G, reducing by 8.7G.

@krishung5
Copy link
Contributor Author

Will create a follow up PR to address @mc-nv's comments.

@krishung5 krishung5 merged commit dfbe63e into main Jul 8, 2024
3 checks passed
@krishung5 krishung5 deleted the krish-reduce-size branch July 8, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants