-
Notifications
You must be signed in to change notification settings - Fork 192
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 CalcJob
s
#5841
Conversation
c69bd6d
to
a9c5369
Compare
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 |
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.
@sphuber Thanks!
I have two minor requests. 1. Could you replace the containerized code test at
aiida-core/tests/engine/processes/calcjobs/test_calc_job.py
Lines 243 to 247 in 76b6ba5
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( |
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.
a9c5369
to
85f776d
Compare
I have added a test (instead of replacing) in the latest commit.
I will ping other developers in Slack to see if they want to have a look at the naming. |
@sphuber Thanks for adding the test. The conflict comes from the newly merged PR where I removed duplicated |
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.
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"
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 thebash -c
command that is run inside the container.For example, to run
pw.x
the submit line needs to look like:To enable this, a new attribute is added to the
JobTemplateCodeInfo
dataclass. This instructs theScheduler
to wrap the executable and all its arguments in quotes. TheAbstractCode
has a new attribute with the same name that isFalse
by default to keep backwards-compatibility but which can be set toTrue
to enable compatibility with Docker.The attribute has an associated getter and setter, which is added to the
AbstractCode
and not theContainerizedCode
because theScheduler
plugin will attempt to retrieve this attribute for all code types. If the properties would be added toContainerizedCode
, the scheduler plugin would except with anAttributeError
.