Skip to content

[feat] Improve Sarus backend #1737

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.

Sign up for GitHub

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

Closed
wants to merge 10 commits into from
Closed

[feat] Improve Sarus backend #1737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3c5591a
add SARUS changes
Feb 2, 2021
feb003f
rm skip_pull in favor of with_metahub
Feb 2, 2021
e17d4ed
tempfile anyone
Feb 2, 2021
346400b
adjust tempdir
Feb 2, 2021
ff868ad
kinda like skip_pull to speed up things
Feb 2, 2021
d7bb025
adjust tests
Feb 3, 2021
6c71a15
add space before cmd
Feb 3, 2021
9a3c0ab
remove empty cmd test for containers
Feb 3, 2021
93f19c9
adjust cmd join in container test
Feb 3, 2021
ac6fd8a
fix unittest
Feb 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions reframe/core/containers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import reframe.core.fields as fields
import reframe.utility.typecheck as typ
from reframe.core.exceptions import ContainerError

import tempfile

class ContainerPlatform(abc.ABC):
'''The abstract base class of any container platform.'''
Expand Down Expand Up @@ -88,9 +88,6 @@ def validate(self):
if self.image is None:
raise ContainerError('no image specified')

if not self.commands:
Copy link
Author

@ChristianKniep ChristianKniep Feb 2, 2021

Choose a reason for hiding this comment

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

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.

raise ContainerError('no commands specified')


class Docker(ContainerPlatform):
'''Container platform backend for running containers with `Docker
Expand Down Expand Up @@ -119,32 +116,57 @@ class Sarus(ContainerPlatform):
#: :default: :class:`False`
with_mpi = fields.TypedField(bool)

#: Using metahub to fetch instance specific images
#:
#: :type: boolean
#: :default: :class:`False`
with_metahub = fields.TypedField(bool)
Copy link
Contributor

@vkarak vkarak Feb 2, 2021

Choose a reason for hiding this comment

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

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

Copy link
Author

@ChristianKniep ChristianKniep Feb 4, 2021

Choose a reason for hiding this comment

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

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?

#: Skipping pull alltogether to speed up runs
#:
#: :type: boolean
#: :default: :class:`False`
skip_pull = fields.TypedField(bool)

def __init__(self):
super().__init__()
self.with_mpi = False
self._command = 'sarus'
self.with_metahub = False
Copy link
Contributor

@teojgo teojgo Feb 3, 2021

Choose a reason for hiding this comment

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

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

self.skip_pull = False

def emit_prepare_commands(self):
# The format that Sarus uses to call the images is
# <reposerver>/<user>/<image>:<tag>. If an image was loaded
# locally from a tar file, the <reposerver> is 'load'.
if self.image.startswith('load/'):
if self.image.startswith('load/') or self.skip_pull:
return []

return [self._command + ' pull %s' % self.image]
# Using
if self.with_metahub:
tmpdir = tempfile.mkdtemp()
return [
'docker pull -q mh.qnib.org/'+self.image,
'docker save -o '+tmpdir+'/image.tar mh.qnib.org/'+self.image,
Comment on lines +147 to +148
Copy link
Contributor

@vkarak vkarak Feb 2, 2021

Choose a reason for hiding this comment

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

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

Copy link
Author

@ChristianKniep ChristianKniep Feb 4, 2021

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

@teojgo teojgo Feb 4, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@teojgo teojgo Feb 4, 2021

Choose a reason for hiding this comment

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

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

self._command + ' load ${TMP_DIR}/gromacs.tar mh.qnib.org/'+self.image,
'rm -rf '+tmpdir
]
else:
return [self._command + ' pull %s' % self.image]

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

@teojgo teojgo Feb 3, 2021

Choose a reason for hiding this comment

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

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]

mp for mp in self.mount_points]
run_opts.append('--workdir='+self.workdir)
Copy link
Author

@ChristianKniep ChristianKniep Feb 2, 2021

Choose a reason for hiding this comment

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

Cleaner then just changing the dir with cd

Copy link
Contributor

@teojgo teojgo Feb 3, 2021

Choose a reason for hiding this comment

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

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

if self.with_mpi:
run_opts.append('--mpi')

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),
Copy link
Author

@ChristianKniep ChristianKniep Feb 2, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@teojgo teojgo Feb 3, 2021

Choose a reason for hiding this comment

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

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

self.image)
return run_cmd + "'" + '; '.join(
['cd ' + self.workdir] + self.commands) + "'"
if self.commands:
return run_cmd + " '" + '; '.join(self.commands) + "'"
Copy link
Contributor

@teojgo teojgo Feb 3, 2021

Choose a reason for hiding this comment

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

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.

else:
return run_cmd


class Shifter(Sarus):
Expand Down
43 changes: 24 additions & 19 deletions unittests/test_containers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,20 @@ def expected_cmd_mount_points(container_variant):
return ('sarus run '
'--mount=type=bind,source="/path/one",destination="/one" '
'--mount=type=bind,source="/path/two",destination="/two" '
"image:tag bash -c 'cd /stagedir; cmd1; cmd2'")
'--workdir=/stagedir '
"image:tag 'cmd1; cmd2'")
elif container_variant == 'Sarus+mpi':
return ('sarus run '
'--mount=type=bind,source="/path/one",destination="/one" '
'--mount=type=bind,source="/path/two",destination="/two" '
"--mpi image:tag bash -c 'cd /stagedir; cmd1; cmd2'")
'--workdir=/stagedir '
"--mpi image:tag 'cmd1; cmd2'")
elif container_variant == 'Sarus+localimage':
return ('sarus run '
'--mount=type=bind,source="/path/one",destination="/one" '
'--mount=type=bind,source="/path/two",destination="/two" '
"load/library/image:tag bash -c 'cd /stagedir; cmd1; cmd2'")
'--workdir=/stagedir '
"load/library/image:tag 'cmd1; cmd2'")
elif container_variant == 'Singularity':
return ('singularity exec -B"/path/one:/one" -B"/path/two:/two" '
"image:tag bash -c 'cd /stagedir; cmd1; cmd2'")
Expand All @@ -67,17 +70,20 @@ def expected_cmd_mount_points(container_variant):
return ('shifter run '
'--mount=type=bind,source="/path/one",destination="/one" '
'--mount=type=bind,source="/path/two",destination="/two" '
"image:tag bash -c 'cd /stagedir; cmd1; cmd2'")
'--workdir=/stagedir '
"image:tag 'cmd1; cmd2'")
elif container_variant == 'Shifter+localimage':
return ('shifter run '
'--mount=type=bind,source="/path/one",destination="/one" '
'--mount=type=bind,source="/path/two",destination="/two" '
"load/library/image:tag bash -c 'cd /stagedir; cmd1; cmd2'")
'--workdir=/stagedir '
"load/library/image:tag 'cmd1; cmd2'")
elif container_variant == 'Shifter+mpi':
return ('shifter run '
'--mount=type=bind,source="/path/one",destination="/one" '
'--mount=type=bind,source="/path/two",destination="/two" '
"--mpi image:tag bash -c 'cd /stagedir; cmd1; cmd2'")
'--workdir=/stagedir '
"--mpi image:tag 'cmd1; cmd2'")


@pytest.fixture
Expand All @@ -98,30 +104,36 @@ def expected_cmd_run_opts(container_variant):
elif container_variant == 'Shifter':
return ('shifter run '
'--mount=type=bind,source="/path/one",destination="/one" '
"--foo --bar image:tag bash -c 'cd /stagedir; cmd'")
'--workdir=/stagedir '
"--foo --bar image:tag 'cmd'")
elif container_variant == 'Shifter+mpi':
return ('shifter run '
'--mount=type=bind,source="/path/one",destination="/one" '
"--mpi --foo --bar image:tag bash -c 'cd /stagedir; cmd'")
'--workdir=/stagedir '
"--mpi --foo --bar image:tag 'cmd'")
elif container_variant == 'Shifter+localimage':
return (
'shifter run '
'--mount=type=bind,source="/path/one",destination="/one" '
"--foo --bar load/library/image:tag bash -c 'cd /stagedir; cmd'"
'--workdir=/stagedir '
"--foo --bar load/library/image:tag 'cmd'"
)
elif container_variant == 'Sarus':
return ('sarus run '
'--mount=type=bind,source="/path/one",destination="/one" '
"--foo --bar image:tag bash -c 'cd /stagedir; cmd'")
'--workdir=/stagedir '
"--foo --bar image:tag 'cmd'")
elif container_variant == 'Sarus+mpi':
return ('sarus run '
'--mount=type=bind,source="/path/one",destination="/one" '
"--mpi --foo --bar image:tag bash -c 'cd /stagedir; cmd'")
'--workdir=/stagedir '
"--mpi --foo --bar image:tag 'cmd'")
elif container_variant == 'Sarus+localimage':
return (
'sarus run '
'--mount=type=bind,source="/path/one",destination="/one" '
"--foo --bar load/library/image:tag bash -c 'cd /stagedir; cmd'"
'--workdir=/stagedir '
"--foo --bar load/library/image:tag 'cmd'"
)
elif container_variant == 'Singularity':
return ('singularity exec -B"/path/one:/one" '
Expand All @@ -145,13 +157,6 @@ def test_missing_image(container_platform):
with pytest.raises(ContainerError):
container_platform.validate()


def test_missing_commands(container_platform):
container_platform.image = 'image:tag'
with pytest.raises(ContainerError):
container_platform.validate()


def test_prepare_command(container_platform, expected_cmd_prepare):
assert container_platform.emit_prepare_commands() == expected_cmd_prepare

Expand Down