-
Notifications
You must be signed in to change notification settings - Fork 32
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
add podman unit tests #312
Conversation
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
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 good!
tasks: | ||
ls: {parameters: {inputs: {}, outputs: {}}} | ||
free: {entrypoint: '/usr/bin/free', parameters: {inputs: {}, outputs: {}}} | ||
""" | ||
|
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.
Two blank lines according to PEP 8.
@@ -50,6 +66,25 @@ def setUp(self) -> None: | |||
def tearDown(self) -> None: | |||
Shell.sync_workspace = self.sync_workspace | |||
|
|||
@unittest.skipUnless(_HAVE_PODMAN, reason="No podman available.") | |||
def test_mlcube_default_entrypoints(self): |
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.
The name should be test_podman_default_entrypoints
. Now, there's another function with the same name that overrides this one.
def test_mlcube_default_entrypoints(self): | ||
with patch("io.open", mock_open(read_data=_MLCUBE_DEFAULT_ENTRY_POINT_PODMAN)): | ||
mlcube: DictConfig = MLCubeConfig.create_mlcube_config( | ||
"/some/path/to/mlcube.yaml", runner_config=Config.DEFAULT, runner_cls=PodmanRun |
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.
Should be runner_cls=DockerRun
. The DockerRun
class can run MLCubes with docker and podman.
DockerRun(mlcube, task=None).configure() | ||
DockerRun(mlcube, task='ls').run() | ||
DockerRun(mlcube, task='pwd').run() | ||
|
||
@unittest.skipUnless(_HAVE_DOCKER, reason="No docker available.") | ||
def test_mlcube_default_entrypoints(self): |
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.
We can rename it to test_docker_default_entrypoints
.
@@ -69,6 +104,25 @@ def test_mlcube_default_entrypoints(self): | |||
DockerRun(mlcube, task='ls').run() | |||
DockerRun(mlcube, task='pwd').run() | |||
|
|||
@unittest.skipUnless(_HAVE_PODMAN, reason="No podman available.") | |||
def test_mlcube_custom_entrypoints(self): |
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.
The name should be test_podman_custom_entrypoints
.
DockerRun(mlcube, task=None).configure() | ||
DockerRun(mlcube, task='ls').run() | ||
DockerRun(mlcube, task='free').run() | ||
|
||
@unittest.skipUnless(_HAVE_DOCKER, reason="No docker available.") | ||
def test_mlcube_custom_entrypoints(self): |
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.
We can rename it to test_docker_custom_entrypoints
.
@@ -27,6 +28,21 @@ | |||
free: {entrypoint: '/usr/bin/free', parameters: {inputs: {}, outputs: {}}} | |||
""" | |||
|
|||
_MLCUBE_DEFAULT_ENTRY_POINT_PODMAN = """ | |||
podman: |
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.
By default, MLCube won't recognize the podman
name here. Since this is actually the platform name, users will need to make sure their system settings file (~/mlcube.yaml
) contains the podman
platform.
I think it will be easier to use docker
here, e.g., docker:
. I think podman
defines the docker
alias, so the docker runner will run just fine. For sake of these tests, it will be better to explicitly instruct the docker runner to use podman. This can be done by overriding the docker
key. Complete configuration will (probably) look like this:
docker:
docker: podman
image: ubuntu:18.04
""" | ||
|
||
_MLCUBE_CUSTOM_ENTRY_POINTS_PODMAN = """ | ||
podman: |
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.
See my comment above regarding podman
in _MLCUBE_DEFAULT_ENTRY_POINT_PODMAN
.
Add podman unit tests, checking for the existence of podman first.