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

[serving] Updates onnxruntime to 1.20.0 and add integration tests #2615

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

xyang16
Copy link
Contributor

@xyang16 xyang16 commented Dec 2, 2024

Description

Brief description of what this PR is about

  • If this change is a backward incompatible change, why must this change be made?
  • Interesting edge cases to note here

@xyang16 xyang16 requested review from zachgk and a team as code owners December 2, 2024 17:15
@@ -16,7 +16,7 @@ ARG djl_version
ARG djl_serving_version
ARG python_version=3.11
ARG djl_torch_version=2.5.1
ARG djl_onnx_version=1.19.0
ARG djl_onnx_version=1.20.0
Copy link
Contributor

Choose a reason for hiding this comment

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

typo? should this be 1.20.1?

Copy link
Contributor Author

@xyang16 xyang16 Dec 2, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realized I have 1.20.1 in title, fixed it.

@@ -81,6 +81,7 @@ RUN apt-get update && apt-get install -yq libaio-dev libopenmpi-dev g++ unzip cu
COPY requirements-lmi.txt ./requirements.txt
RUN pip3 install torch==2.5.1 torchvision==0.20.1 --index-url https://download.pytorch.org/whl/cu124 && pip3 cache purge
RUN pip3 install -r requirements.txt \
&& pip3 install onnxruntime-gpu==$djl_onnx_version \
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this to the requirements-lmi.txt 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.

To make onnxruntime-gpu to work, it needs to be installed after onnxruntime is installed. I found that if I put it in requirements-lmi.txt, the order is not guaranteed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to install both onnxruntime and onnxruntime-gpu? Could we replace onnxruntime in the requirements file with onnxruntime-gpu==1.20.0?

Copy link
Contributor Author

@xyang16 xyang16 Dec 2, 2024

Choose a reason for hiding this comment

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

  1. djl_converter will install onnxruntime as a dependency: https://github.com/deepjavalibrary/djl/blob/master/extensions/tokenizers/src/main/python/requirements.txt#L6. I have moved djl_converter out of requirements-lmi.txt with --no-deps.
  2. Moved onnxruntime-gpu into requirements-lmi.txt

@xyang16 xyang16 changed the title [serving] Updates onnxruntime to 1.20.1 and add integration tests [serving] Updates onnxruntime to 1.20.0 and add integration tests Dec 2, 2024
ARG djl_onnx_version=1.20.0

# djl converter wheel for text-embedding use case
ARG djl_converter_wheel="https://publish.djl.ai/djl_converter/djl_converter-0.31.0-py3-none-any.whl"
Copy link
Contributor

Choose a reason for hiding this comment

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

once we update this wheel to 0.32.0, will we be able to include it in the requirements 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.

I can make some changes to include it in the requirements file

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should be putting all python dependencies in the requirements file so that we can see any dependency conflicts at build time.

The torch dependencies are outside for two reasons:

  • torch by itself is like 5-6gb, so it helps parallelize the image download
  • we're using an index url to fetch the cuda 124 version.

Other than that, we should put deps into the requirements file (torch is also included in the requirements file to ensure no version conflicts with other deps)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because requirements file does not support --no-deps, I have to move djl-convert wheel outside of requirements. I can remove the onnxruntime dependency from djl-convert, so we don't need --no-deps for djl-convert.

@xyang16 xyang16 merged commit e4c1ef8 into deepjavalibrary:master Dec 3, 2024
9 checks passed
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