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

gpu: Update to HPC 22.3, reduced image size #1918

Merged
merged 3 commits into from
Jun 14, 2022

Conversation

kenhester
Copy link
Collaborator

No description provided.

Copy link
Contributor

@georgebisbas georgebisbas left a comment

Choose a reason for hiding this comment

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

Thanks!

#
# docker run --gpus all --rm -it -p 8888:8888 -p 8787:8787 -p 8786:8786 --device=/dev/infiniband/uverbs0 --device=/dev/infiniband/rdma_cm devito:nvidia
#
# to run in user context on a cluster with shared filesystem, you can add the correct user config as docker options e.g.:
# docker run --gpus all --rm -it -v `pwd`:`pwd` -w `pwd` -u $(id -u):$(id -g) devito:nvidia python examples/seismic/acoustic/acoustic_example.py
#
##############################################################
FROM python:3.8
#FROM python:3.9
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the comments to be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was just try to illustrate that there was an alternate non-slim version of the base container. #FROM python:3.9 could be removed, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, the slimmer, the better right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is the ask from some of our the users of the container.

#
# docker run --gpus all --rm -it -p 8888:8888 -p 8787:8787 -p 8786:8786 --device=/dev/infiniband/uverbs0 --device=/dev/infiniband/rdma_cm devito:nvidia
#
# to run in user context on a cluster with shared filesystem, you can add the correct user config as docker options e.g.:
# docker run --gpus all --rm -it -v `pwd`:`pwd` -w `pwd` -u $(id -u):$(id -g) devito:nvidia python examples/seismic/acoustic/acoustic_example.py
#
##############################################################
FROM python:3.8
#FROM python:3.9
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, the slimmer, the better right?

curl -sL https://deb.nodesource.com/setup_12.x | bash - && \
apt-get install -y -q \
nodejs \
# nvhpc-22-3-cuda-multi \
Copy link
Contributor

Choose a reason for hiding this comment

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

same I guess

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not need in most cases. Commented out for a smaller slimmer container

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remind us what's cuda-multi for?

Copy link
Contributor

Choose a reason for hiding this comment

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

Two previous cuda version and everythng that goes with it. Takes a lot of space usually yeah and not needed.

@FabioLuporini FabioLuporini changed the title [gpu, compiler] Updated to HPC 22.3, reduced image size gpu: Update to HPC 22.3, reduced image size May 25, 2022
curl -sL https://deb.nodesource.com/setup_12.x | bash - && \
apt-get install -y -q \
nodejs \
# nvhpc-22-3-cuda-multi \
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remind us what's cuda-multi for?

curl -sL https://deb.nodesource.com/setup_12.x | bash - && \
apt-get install -y -q \
nodejs \
# nvhpc-22-3-cuda-multi \
nvhpc-22-3 \
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry it took a while to get to this PR, my bad... but now that we're here, we could also do 22-5 , right?

@mloubout
Copy link
Contributor

Could we make this automatic somehow with a cron workflow to avoid the effort and PRs. Like just check once a week if there is a new sdk and update the image

@FabioLuporini
Copy link
Contributor

Could we make this automatic somehow with a cron workflow to avoid the effort and PRs. Like just check once a week if there is a new sdk and update the image

That's a lovely idea -- tho that's probably on us ;)

@FabioLuporini
Copy link
Contributor

@mloubout @georgebisbas can you make any sense of the conflict with the existing Dockerfile.nvidia ?

@georgebisbas
Copy link
Contributor

@mloubout @georgebisbas can you make any sense of the conflict with the existing Dockerfile.nvidia ?

I wonder whether this conflict comes from @kenhester 's fork?
@kenhester could you kindly double-check?

@mloubout
Copy link
Contributor

mloubout commented Jun 9, 2022

Conflict is from the changes to dockerfile that were made in between yes since the COPY has slightly changed.

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #1918 (d5f1112) into master (dee1b76) will decrease coverage by 0.75%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1918      +/-   ##
==========================================
- Coverage   89.60%   88.85%   -0.76%     
==========================================
  Files         211      211              
  Lines       36077    36077              
  Branches     5425     5425              
==========================================
- Hits        32326    32055     -271     
- Misses       3248     3506     +258     
- Partials      503      516      +13     
Impacted Files Coverage Δ
tests/test_adjoint.py 26.66% <0.00%> (-73.34%) ⬇️
examples/seismic/tti/wavesolver.py 77.50% <0.00%> (-20.84%) ⬇️
examples/seismic/tti/operators.py 79.42% <0.00%> (-17.29%) ⬇️
examples/seismic/viscoacoustic/wavesolver.py 87.03% <0.00%> (-12.97%) ⬇️
examples/seismic/viscoacoustic/operators.py 87.71% <0.00%> (-12.29%) ⬇️
tests/test_derivatives.py 94.64% <0.00%> (-5.36%) ⬇️
tests/test_dse.py 94.62% <0.00%> (-5.18%) ⬇️
benchmarks/user/benchmark.py 58.57% <0.00%> (-4.29%) ⬇️
examples/seismic/utils.py 71.91% <0.00%> (-2.74%) ⬇️
devito/core/autotuning.py 92.41% <0.00%> (-1.34%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dee1b76...d5f1112. Read the comment docs.

@mloubout
Copy link
Contributor

Should be GTG

@FabioLuporini
Copy link
Contributor

Thanks a lot @kenhester ! Merging, sorry it took a while, we have had a long list of PRs in the queue

@FabioLuporini FabioLuporini merged commit 5a33f2a into devitocodes:master Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants