Container code

[unkcpz]

There are the following major changes in this PR:

• For the job script generated, if there are $ contained in the string, it will not be escaped in single quotes.
• A ContainerCode class is created to represent the code running in the container
• The command-line interface for setting this ContainerCode instant is added

Since the container set is now included in the setting of the container code, which I think for the production using this should be separated, the implementation of the verdi code setup --on-container interface is a bit of a mess, all the options are in the code setup subcommand.

[yakutovicha]

Please see my comments below.

[yakutovicha]

I wonder if "on_container" should be a separate option. Maybe instead of having the "ON_COMPUTER" option we call it "CODE_LOCATION" and provide 3 choices: on a computer, on a container, local. Something like that.

[ltalirz]

I agree that these three cases are related; the downside of your suggestion is that we would need to break backwards compatibility.
At this stage I would rather vote for a separate option that is turned off by default.

[yakutovicha]

Honestly, I am not convinced anymore whether creating a new ContainerCode class is better than adding a few more argument to the standard Code

[ltalirz]

We don't know yet how exactly we want to represent all the container metadata, so I think going with a subclass is the right way for now.

[sphuber]

Honestly, I am not convinced anymore whether creating a new ContainerCode class is better than adding a few more argument to the standard Code

Would you mind sharing why? That route has been tried multiple times (local and remote for Code, mesh and explicit for KpointsData) and it always creates a lot of confusion, both in the code and for the user.

[ltalirz]

thanks @unkcpz - most of this looks good to me, and is in line with what we discussed.

just a couple of comments, mostly on naming

[ltalirz]

Alternative name: Container and ContainerizedCode

I don't have strong preferences for any of these options though, also ContainerCode is fine.

[giovannipizzi]

In this case I'd go for ContainerCode: it makes it clear it's a Code (actually it's a subclass) and it's a bit shorter than ContainerizedCode

[sphuber]

I would personally definitely keep the Code suffix. Also I think ContainerizedCode is more "correct" and therefore for me clearer. I usually don't mind names that are a bit longer if that makes them clearer. But I could also live with ContainerCode if others prefer this.

[ltalirz]

I don't think abbreviations are necessary here.
command_template is perfectly fine and easier to read

[ltalirz]

I propose container_command, in analogy to https://docs.docker.com/engine/reference/run/

[ltalirz]

We don't know yet how exactly we want to represent all the container metadata, so I think going with a subclass is the right way for now.

[ltalirz]

I believe this should default to singularity instead, since it is way more popular.

I would perhaps call this container-engine-command (I don't think "template" needs to be part of the name; we also don't call the work directory "template")

[giovannipizzi]

Note that there is a typo! the first time you say /workir instead of /workdir

[ltalirz]

hm... why?

[giovannipizzi]

I was also originally thinking that the three options are mutually exclusive, but you're right - we could have containerised code that runs both 'on computer' or 'in-db'. @unkcpz did you test the two cases?

[unkcpz]

What is this in-db? it is the other way to say for the local code, no? So either we on_computer or 'not_on_computer' which is local (in-db?) or we on_container?

[ltalirz]

Oh, this fix is way too far-reaching; it will affect any string that contains a dollar sign, not just the one we are looking at.

Can you provide a link to the issue that @giovannipizzi mentioned where they discuss the pros/cons of using single/double quotes?

[giovannipizzi]

I agree this is too much...
Also note the spelling dont -> don't, espace -> escape

The issue is #4836

Shall we add an optional parameter to escape_for_bash, use_double_quotes=False? If True, one should do (untested)

    escaped_quotes = str_to_escape.replace('"', '''"'"'"''')
     return f'"{escaped_quotes}"'

(obvious, no? :-P)

Then, the question however becomes: "who decides which quotes to use?"

Option 1: the new string container-engine-command is escaped with double quotes, the rest with single quotes (backward compatibility). However, this does not address #4836 and it will be complex for users to remember which is quoted how.
Option 2: we also ask one more thing to users: use double quotes instead of single quotes for escaping (default false). Is this too much?

[unkcpz]

I would vote for option 2. But this prompt setup question should be in the computer setup stage just after the scheduler setting there, I assume.

[ltalirz]

Are you changing something in this method?

In a quick browse I didn't see anything specific to the container code (but I might be wrong).
You might be able to subclass CodeEntityLoader.

[sphuber]

As described above, if we simply make the entry point core.code.containerized then load_code can be used and none of this extra code is needed

[ltalirz]

thanks @unkcpz - most of this looks good to me, and is in line with what we discussed.

just a couple of comments, mostly on naming

[giovannipizzi]

Some additional comments to be taken into account.

One important thing: before merging it would be important to test this (both ContainerCode in mode ON_COMPUTER and in mode STORE_AND_UPLOAD) to check it actually works. In can give you a path of a pw code I already prepared in sarus on eiger, if you want to try. You could prepare a simple container with python in it and check that you can upload a python script and run it in mode STORE_AND_UPLOAD.

Actually, it would be good to add integration tests where we actually run one of these codes among the tests? Using docker without MPI as an example? I feel (but I'm not sure) that there should be already examples of this for the add code, and docker should be already available in the GitHub actions.

[giovannipizzi]

Note that there is a typo! the first time you say /workir instead of /workdir

[giovannipizzi]

I was also originally thinking that the three options are mutually exclusive, but you're right - we could have containerised code that runs both 'on computer' or 'in-db'. @unkcpz did you test the two cases?

[giovannipizzi]

I agree this is too much...
Also note the spelling dont -> don't, espace -> escape

The issue is #4836

Shall we add an optional parameter to escape_for_bash, use_double_quotes=False? If True, one should do (untested)

    escaped_quotes = str_to_escape.replace('"', '''"'"'"''')
     return f'"{escaped_quotes}"'

(obvious, no? :-P)

Then, the question however becomes: "who decides which quotes to use?"

Option 1: the new string container-engine-command is escaped with double quotes, the rest with single quotes (backward compatibility). However, this does not address #4836 and it will be complex for users to remember which is quoted how.
Option 2: we also ask one more thing to users: use double quotes instead of single quotes for escaping (default false). Is this too much?

[giovannipizzi]

Is this really needed since ContainerCode is a subclass? (maybe it is, I didn't check the underlying code). Same question in at least another point below.

[sphuber]

Whether it is a sub-class in Python doesn't matter. What matters is the structuring of the entry points, because that is what the query builder uses. So as long as Code = core.code and ContainerCode = core.code.container, then querying for all Code's will also match all ContainerCode's, unless subclassing is explicitly turned off. Note that the load_node call here should just be replaced with load_code which is the standard for querying for codes.

[giovannipizzi]

This is completely overriding the behaviour above? I.e., this e.g. is ignoring the behaviour of this_withmpi and not putting the mpi_args, that I think is incorrect?

[giovannipizzi]

Let's check Leo's suggestion for the name, above (I liked it). In any case, let's use the same name for the function and the attribute, to avoid confusion in users when they call it via the API or when they query for it via the QueryBuilder. Finally, for consistency, let's add get_ in front. So, e.g., container_engine_command for the attribute and get_container_engine_command for the method

[giovannipizzi]

to be adapted according to my comment above (and testing both single-quoted escaping, i.e. now, and double-quoted escaping - essentially copying the tests an swapping all single quote with double quotes and vice versa

[giovannipizzi]

There are already quite a few reviews, but I added @sphuber if he wants to have an eye (not critical if he doesn't have time) since there are quite a few things to be careful about (entry point names, subclassing, ...)

[sphuber]

Thanks @unkcpz . I have given this a quick read, but I haven't really gone into depth of the ContainerCode implementation for now, since I haven't been present during the meetings. So I trust the implementation is ok.

The main problem that I see is that we are making the already complex verdi code setup even more complex. The fact that this endpoint is used for three different classes feels a bit problematic. We have already had plenty of problems with the command merely supporting local and remote codes, but I feel this will increase the complexity signifcantly. Now I am not sure if we can or should address this now, but I think that in the future an approach more in the sense of the transport commands, where each plugin can provide their own CLI subcommand, would be the way to go. That solution is not without its own problems for sure, but I think that will be better in the end.

[sphuber]

I would personally definitely keep the Code suffix. Also I think ContainerizedCode is more "correct" and therefore for me clearer. I usually don't mind names that are a bit longer if that makes them clearer. But I could also live with ContainerCode if others prefer this.

[sphuber]

Honestly, I am not convinced anymore whether creating a new ContainerCode class is better than adding a few more argument to the standard Code

Would you mind sharing why? That route has been tried multiple times (local and remote for Code, mesh and explicit for KpointsData) and it always creates a lot of confusion, both in the code and for the user.

[sphuber]

Suggested change

	ON_CONTAINER = 'on container'
	ON_CONTAINER = 'on container'

Would go with CONTAINERIZED or CONTAINER here.

[sphuber]

As described above, if we simply make the entry point core.code.containerized then load_code can be used and none of this extra code is needed

[ltalirz]

@unkcpz What is the status of this?

I guess the 2.0 release candidate is due by Jan 15th, would be good to get this in?

[unkcpz]

Thanks for headup @ltalirz, I am cleaning up the commits and working on this today.

[unkcpz]

Got the error in CI https://github.com/aiidateam/aiida-core/runs/4765450356?check_suite_focus=true:

verdi -p test_${AIIDA_TEST_BACKEND} run ${SYSTEM_TESTS}/test_daemon.py
Running the `ArithmeticAddCalculation`
Warning: could not parse scheduler output: the `_scheduler-stderr.txt` file is missing
Warning: could not parse scheduler output: the `_scheduler-stdout.txt` file is missing
Warning: output parser returned exit code<310>: The output file could not be read.

It only appears in this branch, not sure which file change causing this. I can reproduce it in my local machine.

EDIT: By rollback escaping change this error is fixed although the _aiidasubmit.sh generated by two branches are all the same, very strange.

[unkcpz]

Another issue that block me to finish this PR is that to using docker as an example to configure the add code and run in containerized code, the redirection of input and output are confused. The symbol < and > to redirect the input and output cannot be used in the containized command. That is to say, we can not run

docker run -it -v $PWD:/workdir:ro ubuntu /bin/bash < aiida.in > aiida.out

But

docker run -it -v $PWD:/workdir:ro ubuntu sh -c '/bin/bash < aiida.in > aiida.out'

This also happened if we want (and we should) to keep on using the > and < in job script.

I am thinking maybe it is always safe to put the command without containized code after sh -c (not tested yet)? for example for command:

'mpirun' '-np' '1' '/home/aiida/.conda/envs/quantum-espresso-6.7/bin/pw.x' '-in' 'aiida.in'  > 'aiida.out'

When switch to containerized code we have:

<containerized specific command> 'sh' '-c' 'mpirun -np 1 /home/aiida/.conda/envs/quantum-espresso-6.7/bin/pw.x -in aiida.in  > aiida.out'

[ltalirz]

I think you are correct that you will need extra quotes around the command that is to be run inside the container.

You don't necessarily need to remove the quotes around the individual components - one could also escape them as in

<containerized specific command> '\'mpirun\' \'-np 1\' \'/home/aiida/.conda/envs/quantum-espresso-6.7/bin/pw.x\' \'-in\' \'aiida.in\'  > \'aiida.out\''

if that's easier (isn't there a way in AiiDA to do this automatically?)

[ltalirz]

Hm... upon closer look: what about the -a option in docker run?
https://docs.docker.com/engine/reference/run/#foreground
https://stackoverflow.com/a/50161815/1069467

[unkcpz]

Hm... upon closer look: what about the -a option in docker run?

Thanks @ltalirz. Yes, I saw that, there are ways to pass variables instead of using redirections <. We can also do:

docker run -it -v $PWD:/workdir:rw -w /workdir ubuntu /bin/bash  aiida.in > aiida.out

However, if I understand it correctly, it is defined in the calcjob plugins how to pass the input, either use -in for PwCalcJob or < for Add CalcJob. In principle, I think we should not override this.

[ltalirz]

Yes, sorry, forget what I said (I was a bit tired already ;-) ).

I think it is the right approach to have the directory mounted and, from thereon, make sure that the stdout/stderr redirection to files happens inside the container.

[ltalirz]

@unkcpz are you still waiting for something in particular with this PR?

[unkcpz]

Yes, I am waiting for #5350 get merged. More commits are now based on this PR and in my forked repo.

[ltalirz]

Hey @unkcpz , now that #5350 has been merged, would you be able to update this PR?

I think container support is a key feature for 2.0

[unkcpz]

Hi @ltalirz, thanks for the head up. I have other stuff in my hands and already have containerized code PR to finish in my schedule but after Thursday this week. Will take some time to focus on this then.

[sphuber]

@unkcpz think this PR can be closed, correct? Majority of functionality has been merged in other PRs and the final addition of the code plugins is now taken care of in #5507

[unkcpz]

@sphuber yes. I close it. Thanks for head up.