-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
added new GPU docker files #3408
Conversation
COPY --from=build \ | ||
/opt/LightGBM/lightgbm \ | ||
/opt/LightGBM/lib_lightgbm.so \ | ||
/opt/LightGBM/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we apply this patch for this image?
LightGBM/docker/gpu/dockerfile.gpu
Lines 59 to 61 in df37bce
# Add OpenCL ICD files for LightGBM | |
RUN mkdir -p /etc/OpenCL/vendors && \ | |
echo "libnvidia-opencl.so.1" > /etc/OpenCL/vendors/nvidia.icd |
https://github.com/microsoft/LightGBM/pull/3408/files#diff-1dd8c717ecdb4eb66c585ba89bbc2649R69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For distroless image: The file should be included in the nvidia/opencl:devel
base image already, so in Line 69 I just simply copied the file:
COPY --from=build /etc/OpenCL/vendors/nvidia.icd /etc/OpenCL/vendors/nvidia.icd
For opencl image: The file should be present in the nvidia/opencl:runtime
base image that's used in the final stage.
The patch is necessary for the dockerfile based on nvidia/cuda
because it doesn't have all the OpenCL stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Thanks for the detailed response!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great to me! I won't leave a ✔️ because I don't have an environment to test this, but overall I'm ok with it. Exciting addition!
docker/gpu/dockerfile-cli-only.gpu
Outdated
ENV LANG C.UTF-8 | ||
ENV LC_ALL C.UTF-8 | ||
|
||
LABEL maintainer=rs7wz@virginia.edu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this code is being brought into LightGBM, I think that this label should be removed. If users have issues with this Dockerfile, it'll be LightGBM maintainers fixing it, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, please feel free to edit the Dockerfile! (It was copied directly from our repo.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused by this line just like you @jameslamb! But I googled a little bit, and it seems that in Docker world maintainer
means author
...
The MAINTAINER instruction sets the Author field of the generated images.
https://docs.docker.com/engine/reference/builder/#maintainer-deprecated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh ok, I see. I think that we can just remove the label completely. I just don't want someone with an issue to send an email there instead of opening an issue in this repo, especially since after this PR is merge we might make other changes as LightGBM evolves
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, removed in a9d85c3 (also replaced tabs with whitespaces).
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Closed #3286.
Replaces #3287 due to CLA problems.
@rsdmse Thank you very much for your contribution! Please help to review this PR.