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

Add support for Docker images to use as Code for CalcJobs #5841

Merged
merged 5 commits into from
Mar 7, 2023

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 15, 2022

Fixes #5818

The ContainerizedCode is currently not quite compatible with Docker's containerization technology. The reason is that the executable, including all of its command line arguments and the file descriptor redirections need to be part of a single quoted string, passed to the bash -c command that is run inside the container.

For example, to run pw.x the submit line needs to look like:

docker run -i {image_name} sh -c "pw.x -input input.in"

To enable this, a new attribute is added to the JobTemplateCodeInfo dataclass. This instructs the Scheduler to wrap the executable and all its arguments in quotes. The AbstractCode has a new attribute with the same name that is False by default to keep backwards-compatibility but which can be set to True to enable compatibility with Docker.

The attribute has an associated getter and setter, which is added to the AbstractCode and not the ContainerizedCode because the Scheduler plugin will attempt to retrieve this attribute for all code types. If the properties would be added to ContainerizedCode, the scheduler plugin would except with an AttributeError.

@sphuber sphuber force-pushed the feature/5818/support-docker branch 2 times, most recently from c69bd6d to a9c5369 Compare December 20, 2022 16:34
@unkcpz
Copy link
Member

unkcpz commented Jan 5, 2023

Hi @sphuber, is this ready? I am happy to review it.

@sphuber
Copy link
Contributor Author

sphuber commented Jan 16, 2023

Hi @sphuber, is this ready? I am happy to review it.

Yes, I think this is ready and is working. But maybe we should still have a more high-level discussion with people to see if this is the right approach. I don't see another one that would allow to add support for Docker without changing things a lot, so I would be tempted to say this approach is ok as it is not disruptive and adds important functionality, albeit that it complicates the concept of the Computer/Code concepts even further.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

@sphuber Thanks!
I have two minor requests. 1. Could you replace the containerized code test at

def test_containerized_code(file_regression, aiida_localhost):
"""Test the :class:`~aiida.orm.nodes.data.code.containerized.ContainerizedCode`."""
aiida_localhost.set_use_double_quotes(True)
engine_command = """singularity exec --bind $PWD:$PWD {image_name}"""
containerized_code = orm.ContainerizedCode(
with the docker? I'd curious to see the real effect of the wrap.
2. the name wrap_cmdline_params at first glimpse seems in the same type with cmdline_params and prepend_cmdline_params, maybe wrap_cmdline_params_up or wrap_cmdline_params_in_double_quotes to distinguish with others? This request is only personal preference, you can keep it as it is.

The `ContainerizedCode` is currently not quite compatible with Docker's
containerization technology. The reason is that the executable,
including all of its command line arguments and the file descriptor
redirections need to be part of a single quoted string, passed to the
`bash -c` command that is run inside the container.

For example, to run `pw.x` the submit line needs to look like:

    docker run -i {image_name} sh -c "pw.x -input input.in"

To enable this, a new attribute is added to the `JobTemplateCodeInfo`
dataclass. This instructs the `Scheduler` to wrap the executable and all
its arguments in quotes. The `AbstractCode` has a new attribute with the
same name that is `False` by default to keep backwards-compatibility
but which can be set to `True` to enable compatiblity with Docker.

The attribute has an associated getter and setter, which is added to the
`AbstractCode` and not the `ContainerizedCode` because the `Scheduler`
plugin will attempt to retrieve this attribute for all code types. If
the properties would be added to `ContainerizedCode`, the scheduler
plugin would except with an `AttributeError`.
Realized also that the `Computer` should define
`use_double_quotes=False` which is now added to the docs.
@sphuber sphuber force-pushed the feature/5818/support-docker branch from a9c5369 to 85f776d Compare January 30, 2023 21:00
@sphuber
Copy link
Contributor Author

sphuber commented Jan 30, 2023

Could you replace the containerized code test at ... with the docker? I'd curious to see the real effect of the wrap.

I have added a test (instead of replacing) in the latest commit.

the name wrap_cmdline_params at first glimpse seems in the same type with

I will ping other developers in Slack to see if they want to have a look at the naming.

@unkcpz
Copy link
Member

unkcpz commented Jan 31, 2023

@sphuber Thanks for adding the test. The conflict comes from the newly merged PR where I removed duplicated use_double_quotes. I resolve it with a commit, hope you don't mind.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

It is all good from my side. I also give it a try for the oncvpsp code. I find it is very helpful if the image's default user is root and using the option -u $(id -u ${USER}):$(id -g ${USER}) for the docker run, the output file belongs to user and can be deleted by the user with cleanworkdir. But for me, it is just a bonus set as a docker user, feel free to add it to doc or I think it is ready to be merged.

Here is an example of my docker run:

docker run -i -v $PWD:/workdir:rw -w /workdir -u $(id -u ${USER}):$(id -g ${USER}) pspgen/oncvpsp:main sh -c "oncvpsp.x < Li-s-with-d-matteo.in > Li-s-with-d-matteo.out"

@sphuber sphuber merged commit 35f95a7 into aiidateam:main Mar 7, 2023
@sphuber sphuber deleted the feature/5818/support-docker branch March 7, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ContainerizedCode: Add support for running codes within Docker containers
2 participants