aiida-core icon indicating copy to clipboard operation
aiida-core copied to clipboard

containerized code only for remote mpi mapping hpc type

Open unkcpz opened this issue 1 year ago • 6 comments

This is the implementation separated from #5507.

  • [x] The singularity nightly test is added and passed.
  • [x] Also test for the portable type containerized code I have it implemented but better to be merged here after https://github.com/aiidateam/aiida-core/pull/5669

unkcpz avatar Sep 26 '22 16:09 unkcpz

Fix the nightly test for portable containerized code, I find it can be very useful. I think we are very close to finishing this. @sphuber can you have a look at this and then I'll merge and update the document from #5507

unkcpz avatar Sep 28 '22 13:09 unkcpz

Just keep as a note here, while I prepare the nightly test for portable containerized code, a python script using numpy to do the operation. I find it might be useful if the use case can be more generalized, where the current portable containerized code requires setting the script and the executable and it binds to the code entity. There can be a way that the container provides the environment and the executable is python, the script can be then passed as input.

So it is basically an environment-specific running code but needs some general CalcJob to support.

unkcpz avatar Sep 28 '22 13:09 unkcpz

@sphuber thanks a lot for the review.

I would just really change the term image to image_name since it really just references the image name.

I change all the "image" to "image_name" including the template of the engine_command. The other reason I didn't change it was I thought "image" is more general to include the exact image name and also other formats such as the path to .sif file the URL of dockerhub link etc. But I think the image name is also very general and good to be used here.

Can you fix that and then add the docs?

Added.

unkcpz avatar Oct 04 '22 15:10 unkcpz

@sphuber @ltalirz, can you find time to have a look at the documentation I added?

unkcpz avatar Oct 13 '22 10:10 unkcpz

Thanks a lot @unkcpz !

After discussing with @unkcpz in some detail, I believe we both came to the conclusion that there is currently no strong use case for the "portable" containerized code. If the use case is to have a handful of static files and executables, that is easily done by updating the container image. If it is about a more dynamic generation of different scripts, then setting up a separate code for each new version of the scripts is not the solution (and this use case is already covered by aiida-dynamic-workflows).

We therefore agree that the "portable" version of the containerized code can be deleted, reducing both the maintenance burden on our side, as well as making terminology easier to understand for users [1].

@sphuber What do you think?

[1] E.g., for containerized codes, "installed" can easily be misunderstood as meaning that the container image is installed on the computer. However, the distinction of whether the image is present or not is actually not made at all in this implementation (which is fine to me). In this sense, both "installed" and "portable" containerized codes are in fact equally portable - both of them can run on any computer that has the required image & container runtime.

To be honest, I haven't really had the chance to experiment with this myself and I haven't really worked a lot with containers myself, so I didn't critically review the split in that much detail. I assumed that it apparently made sense and that these were two different use-cases. But as you explain it now, it makes perfect sense to only support the single form for now. We can always add on later if it turns out this really is useful. So all good for me to reduce the complexity :+1:

sphuber avatar Oct 14 '22 20:10 sphuber

@ltalirz thanks! I am so desperate to admit the portable containerized code can be removed. I really spend time in bring up the fake use cases. But you are right, the aiida-dynamic-workflow with the containerized installed code is the correct way for what I thought to realize.

I remove the portable container code and update the doc upon your review.

unkcpz avatar Oct 17 '22 17:10 unkcpz

@ltalirz I have discussed a final time with @unkcpz and we think that the current implementation is good enough to be merged. We just made one final simplification and named the plugin simply ContainerizedCode with entry point core.code.containerized (there was no need to keep the Installed infix).

I have also taken your suggestions for the docs into account and have simplified the instructions. It is true that the process of setting up the code and using it, is no different from the other code types (which was the whole point of the design) so it should be integrated neatly into the "How-to run a code" section. Nevertheless, the setup will require some more detailed information specific to containerized codes, such as what technologies are supported and how to define the engine_command settings for each of those. I have currently added those to the ContainerizedCode topic.

I was planning to add a reference in the "How to run a code" section, but I realized that that documentation still refers to the deprecated code setup verdi code setup. I will submit a PR soon after this, to update that section where I will then also seamlessly integrate the containerized option. So I think for now the docs in this PR are good enough. Let me know what you think @unkcpz

sphuber avatar Oct 18 '22 12:10 sphuber

Note that at some point, we will have to update the topic section with more detailed information for each of the supported containerization technologies, such as examples of how to define the engine_command. For now, we just provide the example based on Singularity

sphuber avatar Oct 18 '22 12:10 sphuber

Cheers, I'll give a quick last check now then.

named the plugin simply ContainerizedCode with entry point core.code.containerized

I would have suggested exactly the same

ltalirz avatar Oct 18 '22 12:10 ltalirz

Ok, great I think this should be ready for a final review then @ltalirz I think we addressed all your final comments.

sphuber avatar Oct 18 '22 15:10 sphuber