docker: revamp base deployment #2162

mloubout · 2023-07-10T12:36:41Z

Revamp base deployment not to rebuild nvhpc. Reduces the number of jobs and should reduce CI time since all extras are mostly an env var on top of the existing image. Little bit more verbose but I don't think it's really an issue there
Added icx as a separate image
Fix MPI flags for oneapi

codecov · 2023-07-10T13:03:40Z

Codecov Report

Merging #2162 (9f608a8) into master (c5fe4e6) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #2162      +/-   ##
==========================================
- Coverage   87.06%   87.04%   -0.03%     
==========================================
  Files         223      223              
  Lines       39931    39946      +15     
  Branches     5174     5177       +3     
==========================================
+ Hits        34765    34769       +4     
- Misses       4589     4598       +9     
- Partials      577      579       +2

Impacted Files	Coverage Δ
devito/arch/compiler.py	`38.72% <20.00%> (-0.63%)`	⬇️
conftest.py	`84.04% <33.33%> (ø)`
devito/core/autotuning.py	`93.91% <100.00%> (ø)`
devito/types/dense.py	`94.02% <100.00%> (+<0.01%)`	⬆️
devito/types/dimension.py	`94.32% <100.00%> (+<0.01%)`	⬆️
examples/seismic/source.py	`83.33% <100.00%> (ø)`

docker/Dockerfile.cpu

docker/Dockerfile.nvidia

docker/Dockerfile.amd

ggorman

GTO once green and comments addressed.

ggorman · 2023-07-13T21:27:26Z

docker/Dockerfile.devito

@@ -18,6 +23,7 @@ RUN python3 -m venv /venv && \
    /venv/bin/pip install --no-cache-dir --upgrade pip && \
    /venv/bin/pip install --no-cache-dir jupyter && \
    /venv/bin/pip install --no-cache-dir wheel && \
+    eval "$MPI4PY_FLAGS /venv/bin/pip install --no-cache-dir mpi4py" && \


Where does MPI4PY_FLAGS come from? A quick google indicates zero hits. Add link/comment?

It's just a made up one I made to handle all our different compilers and arches

ggorman · 2023-07-13T21:32:04Z

requirements-optional.txt

+pyrevolve
+scipy


Do we need to update any install instructions - what happens when new users try the tutorials? Will they know that these requirements need to be installed?

Install instruvtions should still be the same and examples should always be installed with .[extras] these were leftover that should have been moved here

requirements-testing.txt

mloubout · 2023-07-14T12:58:24Z

conftest.py

@@ -21,6 +21,11 @@
    MPI = None


+def pytest_collectstart(collector):
+    if collector.fspath and collector.fspath.ext == '.ipynb':
+        collector.skip_compare += ('text/latex',)


see https://nbval.readthedocs.io/en/latest/#Skipping-certain-output-types

This just avoid failures for different sympy version printing slightly different latex

Also needed to be moved to root directory so that conftest is found when testing the examples

Good to know, so this cancels out my LATEX changes in the other PR?

review-notebook-app · 2023-07-15T15:07:08Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

FabioLuporini · 2023-07-17T08:03:16Z

.github/workflows/docker-bases.yml

+          password: ${{ secrets.DOCKER_PASSWORD }}
+
+      - name: cleanup
+        run: docker system prune -a -f


do we actually need to prune everything? can we not just prune ourselves?

this might kill for example my local docker images 😬

It should not touch your images only the 'action" user one, always been there.

wait, I was pretty sure "docker system prune" were a system-level action, that is as if it were issued by the docker group?

FabioLuporini · 2023-07-17T08:04:54Z

.github/workflows/pytest-core-mpi.yml

+      runs-on: ubuntu-latest
+      strategy:
+        matrix:
+          arch: [gcc, icc, icx]


I think we can drop icc and just focus on icx (and clearly gcc)

Thought so too wasn't sure if we wanted to keep it until intel drop it

I would agree to drop it. Worst case scenario if someone is so eager to use icc by default, we could always point out to use the latest stable release

FabioLuporini · 2023-07-17T08:06:36Z

devito/arch/compiler.py

@@ -707,6 +709,17 @@ def __init__(self, *args, **kwargs):
            if mpi_distro != 'IntelMPI':
                warning("Expected Intel MPI distribution with `%s`, but found `%s`"
                        % (self.__class__.__name__, mpi_distro))
+            self.cflags.insert(0, '-cc=%s' % self.CC)
+
+    def get_version(self):


who calls this one?

codepy does, it's basically copied from there but making sure the -cc doesn't make it error

ah ok. Could use a small comment, but that's fine

devito/arch/compiler.py

FabioLuporini · 2023-07-17T08:09:16Z

devito/types/dimension.py

@@ -321,7 +321,8 @@ def _arg_check(self, args, size, interval):
                upper = interval.upper
            else:
                # Autopadding causes non-integer upper limit
-                upper = interval.upper.subs(args)
+                from devito.symbolics import normalize_args


sooner or later we will have to fix these cycling imports

FabioLuporini · 2023-07-17T08:10:31Z

docker/Dockerfile.amd

 ########################################################################
 # HIPCC for GPUs (HIP)
 ########################################################################
 # This will only trigger if arch is hip since the final stage depends on it
 FROM sdk-base as hip

+# Install OpenMPI


why was OpenMPI expanded out ? there must be a logic, but I'm missing it

ah, for CC=, CXX= etc...

Yeah, decided to go concervative and force all the compiler for openmpi we can revisit

FabioLuporini · 2023-07-17T08:12:17Z

requirements-optional.txt

+pyrevolve
+scipy
+distributed


FabioLuporini · 2023-07-17T08:12:26Z

requirements-testing.txt

+flake8>=2.1.0
+nbval
+scipy


FabioLuporini · 2023-07-17T08:12:40Z

requirements.txt

@@ -12,4 +9,4 @@ codepy>=2019.1
 click<9.0
 multidict
 anytree>=2.4.3,<=2.9.0
-distributed<2023.8
+cloudpickle


fantastic 👍

georgebisbas · 2023-07-17T11:02:04Z

examples/seismic/source.py

-        self.stop = stop
-        self.step = step
-        self.num = num
+        self.start = float(start)


.github/workflows/docker-bases.yml

FabioLuporini · 2023-07-17T13:14:42Z

.github/workflows/pytest-core-mpi.yml

@@ -25,6 +25,7 @@ jobs:
    env:
      DEVITO_LANGUAGE: "openmp"
      DEVITO_ARCH: "gcc-9"
+      OMP_NUM_THREADS: "1"


in retrospect, I'm not sure about this one. Don't we want to test the numerical accuracy of MPI+OpenMP tests

FabioLuporini · 2023-07-17T13:15:20Z

devito/arch/compiler.py

@@ -707,6 +709,17 @@ def __init__(self, *args, **kwargs):
            if mpi_distro != 'IntelMPI':
                warning("Expected Intel MPI distribution with `%s`, but found `%s`"
                        % (self.__class__.__name__, mpi_distro))
+            self.cflags.insert(0, '-cc=%s' % self.CC)
+
+    def get_version(self):


ah ok. Could use a small comment, but that's fine

FabioLuporini · 2023-07-17T13:39:27Z

docker/README.md

@@ -10,9 +10,9 @@ Devito provides several images that target different architectures and compilers

 We provide two CPU images:
 - `devito:gcc-*` with the standard GNU gcc compiler.
- `devito:icc-*` with the Intel C compiler for Intel architectures.
+- `devito:icx-*` with the Intel C compiler for Intel architectures.


FabioLuporini

Fantastic. GTG for me

mloubout added dependencies Pull requests that update a dependency file CI continuous integration installation labels Jul 10, 2023

mloubout requested a review from ggorman July 10, 2023 12:36

mloubout force-pushed the docker-base-fast branch from d68ad3b to af219b6 Compare July 10, 2023 12:39

mloubout force-pushed the docker-base-fast branch 22 times, most recently from 304b3a7 to 00b2eb5 Compare July 12, 2023 13:47

georgebisbas reviewed Jul 12, 2023

View reviewed changes

docker/Dockerfile.cpu Show resolved Hide resolved

docker/Dockerfile.cpu Show resolved Hide resolved

docker/Dockerfile.nvidia Show resolved Hide resolved

mloubout force-pushed the docker-base-fast branch from 00b2eb5 to 3bcd0b2 Compare July 12, 2023 14:44

docker: new tags and compiler cleanup

ed72d78

mloubout force-pushed the docker-base-fast branch 3 times, most recently from 8da0f96 to fae6d62 Compare July 13, 2023 19:12

deps: fix dependencies

10b8a48

mloubout force-pushed the docker-base-fast branch from fae6d62 to 10b8a48 Compare July 13, 2023 19:12

ggorman reviewed Jul 13, 2023

View reviewed changes

docker/Dockerfile.amd Show resolved Hide resolved

ggorman requested changes Jul 13, 2023

View reviewed changes

mloubout commented Jul 14, 2023

View reviewed changes

mloubout force-pushed the docker-base-fast branch 2 times, most recently from a0f2f15 to aad6f20 Compare July 15, 2023 15:07

mloubout force-pushed the docker-base-fast branch 2 times, most recently from 2f32636 to ae615c1 Compare July 16, 2023 16:22

FabioLuporini reviewed Jul 17, 2023

View reviewed changes

mloubout force-pushed the docker-base-fast branch from ae615c1 to 066d7c0 Compare July 17, 2023 10:59

georgebisbas reviewed Jul 17, 2023

View reviewed changes

examples/seismic/source.py

self.stop = stop

self.step = step

self.num = num

self.start = float(start)

Copy link

Contributor

georgebisbas Jul 17, 2023

Choose a reason for hiding this comment

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

I see

mloubout force-pushed the docker-base-fast branch from 066d7c0 to 8f3ceb0 Compare July 17, 2023 11:02

ci: prevent latex formating errors with nbval

83af5d0

mloubout force-pushed the docker-base-fast branch 3 times, most recently from f54a46a to 68ddd67 Compare July 17, 2023 12:50

FabioLuporini reviewed Jul 17, 2023

View reviewed changes

deps: drop deprecated ICC in favor of ICX

4202291

mloubout force-pushed the docker-base-fast branch from 68ddd67 to 4202291 Compare July 17, 2023 13:28

FabioLuporini reviewed Jul 17, 2023

View reviewed changes

FabioLuporini approved these changes Jul 17, 2023

View reviewed changes

CI: make render group id stable

9f608a8

mloubout merged commit 9466a03 into master Jul 17, 2023

mloubout deleted the docker-base-fast branch July 17, 2023 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker: revamp base deployment #2162

docker: revamp base deployment #2162

mloubout commented Jul 10, 2023

codecov bot commented Jul 10, 2023 •

edited

Loading

ggorman left a comment

ggorman Jul 13, 2023

mloubout Jul 13, 2023

ggorman Jul 13, 2023

mloubout Jul 14, 2023

mloubout Jul 14, 2023

mloubout Jul 14, 2023

georgebisbas Jul 14, 2023

review-notebook-app bot commented Jul 15, 2023

FabioLuporini Jul 17, 2023

mloubout Jul 17, 2023

FabioLuporini Jul 17, 2023

FabioLuporini Jul 17, 2023

FabioLuporini Jul 17, 2023

mloubout Jul 17, 2023

georgebisbas Jul 17, 2023

FabioLuporini Jul 17, 2023

mloubout Jul 17, 2023

FabioLuporini Jul 17, 2023

FabioLuporini Jul 17, 2023

FabioLuporini Jul 17, 2023

FabioLuporini Jul 17, 2023

mloubout Jul 17, 2023

FabioLuporini Jul 17, 2023

FabioLuporini Jul 17, 2023

FabioLuporini Jul 17, 2023

georgebisbas Jul 17, 2023

FabioLuporini Jul 17, 2023

FabioLuporini Jul 17, 2023

FabioLuporini Jul 17, 2023

FabioLuporini left a comment

		pyrevolve
		scipy

docker: revamp base deployment #2162

docker: revamp base deployment #2162

Conversation

mloubout commented Jul 10, 2023

codecov bot commented Jul 10, 2023 • edited Loading

Codecov Report

ggorman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

review-notebook-app bot commented Jul 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2023 •

edited

Loading