Skip to content
This repository was archived by the owner on Aug 7, 2025. It is now read-only.

Conversation

@agunapal
Copy link
Collaborator

@agunapal agunapal commented May 11, 2023

Description

GPU docker was missing nvgpu

Also, noticed that CPU docker image has some missing dependencies: pynvml , wrong version of Pillow

Also, the code to get the TORCH_VERSION gets the latest version and not the version we want.

Fixes #(issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Feature/Issue validation/testing

Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

  • Test A
    Logs for Test A

  • Test B
    Logs for Test B

Checklist:

  • Did you have fun?
  • Have you added tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

@agunapal agunapal requested review from lxning and msaroufim May 11, 2023 21:41
@agunapal
Copy link
Collaborator Author

@lxning The original code was not installing common.txt in case of CPU too. Please suggest if the suggested code is a better approach

if [ "$CUDA_VERSION" ]; then \
python -m pip install --no-cache-dir torch==$TORCH_VER+$CUDA_VERSION torchvision==$TORCH_VISION_VER+$CUDA_VERSION -f https://download.pytorch.org/whl/torch_stable.html; \
# Install the binary with the latest CUDA version support
python ./ts_scripts/install_dependencies.py --cuda $CUDA_VERSION; \
Copy link
Collaborator

Choose a reason for hiding this comment

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

To solve common_gpu.txt installation, you only need ref
https://github.com/pytorch/serve/blob/master/docker/Dockerfile#L75

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lxning There is a problem with the CPU image too. It doesn't have pynvml , torchaudio, torchtext

# Install the CPU binary
else \
python -m pip install --no-cache-dir torch==$TORCH_VER+cpu torchvision==$TORCH_VISION_VER+cpu -f https://download.pytorch.org/whl/torch_stable.html; \
python ./ts_scripts/install_dependencies.py; \
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Member

@msaroufim msaroufim left a comment

Choose a reason for hiding this comment

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

LGTM just a minor nit on the cloned repo

Copy link
Member

@msaroufim msaroufim left a comment

Choose a reason for hiding this comment

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

LGTM just a minor nit on the cloned repo

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #2340 (1a2b99f) into master (8ea1dab) will not change coverage.
The diff coverage is n/a.

❗ Current head 1a2b99f differs from pull request most recent head 8cf4875. Consider uploading reports for the commit 8cf4875 to get more accurate results

@@           Coverage Diff           @@
##           master    #2340   +/-   ##
=======================================
  Coverage   69.39%   69.39%           
=======================================
  Files          77       77           
  Lines        3441     3441           
  Branches       57       57           
=======================================
  Hits         2388     2388           
  Misses       1050     1050           
  Partials        3        3           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@agunapal agunapal merged commit 35fb574 into master May 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants