[feat] Improve Sarus backend #1737

ChristianKniep · 2021-02-02T13:19:52Z

Some changes to improve the use of SARUS #1736 . @Madeeks

jenkins-cscs · 2021-02-02T13:20:02Z

Can I test this patch?

ChristianKniep · 2021-02-02T13:21:57Z

reframe/core/containers.py

        if self.image is None:
            raise ContainerError('no image specified')

-        if not self.commands:


The creator of an images should know best how to execute the command. E.g. for Spack the ENTRYPOINT needs to be honored.
I'd like to argue that a container should also include the execution script.

ChristianKniep

I put some inline comments into the PR to explain what I am doing.

ChristianKniep · 2021-02-02T13:24:02Z

reframe/core/containers.py

    #: :default: :class:`False`
    with_mpi = fields.TypedField(bool)

+    #: Skip pull of images


This violates the reproducibility; thus, not a keeper.
But (being devil's advocate), an image name like qnib/spack-gromacs:latest violates reproducibility as well. :)
To make an image reproducibile you would want to specify an image using the content hash.

docker image inspect qnib/spack-gromacs-shp2:c5n_18xl_off |jq '.[]|.RepoDigests' [ "qnib/spack-gromacs-shp2@sha256:4f5ae272e343395de2259e2745ab24f20a34688343f9f233cec895915f1704f5" ]

and not qnib/spack-gromacs-shp2:c5n_18xl_off

That might be a thing btw. inspecting the actual hash and putting it into the job output as an strict description.
For some images like ubuntu:18.04 you actually specify a manifest list (image index) and not a 'real' image.

ChristianKniep · 2021-02-02T13:27:18Z

reframe/core/containers.py

        super().launch_command()
        run_opts = ['--mount=type=bind,source="%s",destination="%s"' %
                    mp for mp in self.mount_points]
+        run_opts.append('--workdir='+self.workdir)


Cleaner then just changing the dir with cd

ChristianKniep · 2021-02-02T13:27:49Z

reframe/core/containers.py


        run_opts += self.options
-        run_cmd = self._command + ' run %s %s bash -c ' % (' '.join(run_opts),
+        run_cmd = self._command + ' run %s %s' % (' '.join(run_opts),


Again, the entrypoint you specify here (bash -c) might mess with the container image and renders it unusable.

pep8speaks · 2021-02-02T13:58:11Z

Hello @ChristianKniep, Thank you for updating!

In the file reframe/core/containers.py:

Line 143:16: W291 trailing whitespace
Line 149:80: E501 line too long (87 > 79 characters)

Do see the ReFrame Coding Style Guide

Comment last updated at 2021-02-03 10:47:12 UTC

teojgo · 2021-02-02T14:15:42Z

@ChristianKniep make sure you adjust also the unittests

vkarak

I guess we could also extend the Docker backend in a similar way, couldn't we?

vkarak · 2021-02-02T19:53:51Z

reframe/core/containers.py

+    #:
+    #: :type: boolean
+    #: :default: :class:`False`
+    with_metahub = fields.TypedField(bool)


What about replacing this with registry so that we allow users to specify any container registry?

Usually the registry is part of the string that is the image name.
ubuntu is actually docker.io/library/ubuntu:latest
The first part is the registry, the second the org and the last piece the image name.
I might check the prefix and react accordingly.

But in a greater scheme of things the question becomes how do you deal with container images compared to binaries installed on a system?

vkarak · 2021-02-02T20:27:19Z

reframe/core/containers.py

+                'docker pull -q mh.qnib.org/'+self.image,
+                'docker save -o '+tmpdir+'/image.tar mh.qnib.org/'+self.image,


Couldn't sarus pull work here? I don't understand very much the purpose of the save and load.

yes, sarus pull seems to need a API call that the proxy Metahub is not implementing (yet). working with the SARUS folks to fix that soon(-ish).

In this case, you should put this logic on the test as a prerun step. When sarus can pull directly, you just remove the workaround from the particular test.

Btw, you can use skopeo to copy the image directly as a docker-archive and load it with sarus

codecov-io · 2021-02-03T10:48:46Z

Codecov Report

Merging #1737 (ac6fd8a) into master (ab18ed7) will decrease coverage by 0.02%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #1737      +/-   ##
==========================================
- Coverage   87.27%   87.24%   -0.03%     
==========================================
  Files          46       46              
  Lines        7708     7717       +9     
==========================================
+ Hits         6727     6733       +6     
- Misses        981      984       +3

Impacted Files	Coverage Δ
reframe/core/containers.py	`96.66% <80.00%> (-3.34%)`	⬇️

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 ab18ed7...ac6fd8a. Read the comment docs.

teojgo · 2021-02-03T16:35:24Z

reframe/core/containers.py

        super().__init__()
        self.with_mpi = False
        self._command = 'sarus'
+        self.with_metahub = False


This is not needed since you can specify some hooks to be run before each step of the test pipeline.

teojgo · 2021-02-03T16:37:19Z

reframe/core/containers.py


    def launch_command(self):
        super().launch_command()
        run_opts = ['--mount=type=bind,source="%s",destination="%s"' %


Suggested change

run_opts = ['--mount=type=bind,source="%s",destination="%s"' %

run_opts = [f'--mount=type=bind,source="{mp}",destination="{mp}"' for mp in self.mount_points]

teojgo · 2021-02-03T16:38:14Z

reframe/core/containers.py

        super().launch_command()
        run_opts = ['--mount=type=bind,source="%s",destination="%s"' %
                    mp for mp in self.mount_points]
+        run_opts.append('--workdir='+self.workdir)


Suggested change

run_opts.append('--workdir='+self.workdir)

run_opts.append(f'--workdir={self.workdir}')

teojgo · 2021-02-03T16:41:45Z

reframe/core/containers.py

-        return run_cmd + "'" + '; '.join(
-            ['cd ' + self.workdir] + self.commands) + "'"
+        if self.commands:
+            return run_cmd + " '" + '; '.join(self.commands) + "'"


If we are not going to use bash -c then we don't need the single quotes nor we need the ;. I think it's better if the user passes the whole command verbatim.

teojgo · 2021-02-03T16:43:19Z

reframe/core/containers.py


        run_opts += self.options
-        run_cmd = self._command + ' run %s %s bash -c ' % (' '.join(run_opts),
+        run_cmd = self._command + ' run %s %s' % (' '.join(run_opts),


Suggested change

run_cmd = self._command + ' run %s %s' % (' '.join(run_opts),

run_cmd = f"{self._command} run {' '.join(run_opts)} {self.image}"

teojgo · 2021-02-12T12:40:26Z

@ChristianKniep I can take this issue and work on it. Is that fine?

kniec · 2021-02-12T15:01:32Z

@teojgo Sure, let me know if I can help

vkarak · 2021-02-16T09:32:52Z

I am closing this, since it's replaced by #1775.

add SARUS changes

3c5591a

ChristianKniep commented Feb 2, 2021

View reviewed changes

rm skip_pull in favor of with_metahub

feb003f

tempfile anyone

e17d4ed

teojgo added enhancement prio: normal labels Feb 2, 2021

vkarak requested review from rsarm, teojgo and vkarak February 2, 2021 14:16

vkarak assigned ChristianKniep Feb 2, 2021

Christian Kniep added 2 commits February 2, 2021 18:10

adjust tempdir

346400b

kinda like skip_pull to speed up things

ff868ad

vkarak changed the title ~~add SARUS changes~~ [feat] Improve Docker and Sarus backends Feb 2, 2021

vkarak changed the title ~~[feat] Improve Docker and Sarus backends~~ [feat] Improve Sarus backend Feb 2, 2021

vkarak requested changes Feb 2, 2021

View reviewed changes

Christian Kniep added 5 commits February 3, 2021 09:42

adjust tests

d7bb025

add space before cmd

6c71a15

remove empty cmd test for containers

9a3c0ab

adjust cmd join in container test

93f19c9

fix unittest

ac6fd8a

teojgo requested changes Feb 3, 2021

View reviewed changes

vkarak added this to the ReFrame 3.4.2 milestone Feb 8, 2021

vkarak assigned teojgo Feb 8, 2021

vkarak closed this Feb 16, 2021

		'docker pull -q mh.qnib.org/'+self.image,
		'docker save -o '+tmpdir+'/image.tar mh.qnib.org/'+self.image,

	run_opts = ['--mount=type=bind,source="%s",destination="%s"' %
	run_opts = [f'--mount=type=bind,source="{mp}",destination="{mp}"' for mp in self.mount_points]

	run_opts.append('--workdir='+self.workdir)
	run_opts.append(f'--workdir={self.workdir}')

	run_cmd = self._command + ' run %s %s' % (' '.join(run_opts),
	run_cmd = f"{self._command} run {' '.join(run_opts)} {self.image}"

[feat] Improve Sarus backend #1737

[feat] Improve Sarus backend #1737

Uh oh!

Conversation

ChristianKniep commented Feb 2, 2021

Uh oh!

jenkins-cscs commented Feb 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristianKniep left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pep8speaks commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-02-03 10:47:12 UTC

Uh oh!

teojgo commented Feb 2, 2021

Uh oh!

vkarak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 3, 2021

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teojgo commented Feb 12, 2021

Uh oh!

kniec commented Feb 12, 2021

Uh oh!

vkarak commented Feb 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pep8speaks commented Feb 2, 2021 •

edited

Loading