Containerized code: double quotes setting of computer and code setup #5478

unkcpz · 2022-04-08T11:07:20Z

The PR is a part required for containerized code implementation (also for hyperqueue without hacky workaround to evaluate command-line variable by $ properly) and is separated from #5250. It allows users to set up computer and code with an extra option --use-double-quotes to using double quotes for the command run line of job script.

ltalirz · 2022-04-08T11:14:31Z

Thanks @unkcpz . At a high level, I'm just wondering: do we really need to push this choice to the user?

I.e. can't we be clever about using double quotes where necessary (if we detect some environment variable / ...)? This will be yet another option in the computer an code setup and I wonder whether people really need to actively care about this.

unkcpz · 2022-04-08T11:21:42Z

Hi @ltalirz, yes ideally it should be intelligent enough to know when to use double quotes and not. But I think it is hard to decide by what criteria we escape it with double or single quotes. Setting it manually might be a safe way to keep the backward compatibility.

sphuber

Thanks @unkcpz . Mostly minor comments, but one fundamental question: is the separate control of quoting in the Code and Computer necessary? And if so, why should the quoting for the Computer only apply to the argument before the executable and not any that follows, such as the input and output redirection?

aiida/orm/computers.py

sphuber · 2022-04-08T17:48:12Z

aiida/orm/computers.py

+    def get_use_double_quotes(self) -> bool:
+        return self.get_property('use_double_quotes', False)
+
+    def set_use_double_quotes(self, val: bool) -> None:
+        self.set_property('use_double_quotes', bool(val))


Instead of type coercion, I think you should check the type.

from aiida.common.lang import type_check type_check(val, bool)

Thanks, changed.

aiida/manage/tests/pytest_fixtures.py

sphuber · 2022-04-08T17:50:41Z

aiida/orm/nodes/data/code.py

+        """
+        Set whether a code cmdline is escape with double quotes.
+        """


Suggested change

"""

Set whether a code cmdline is escape with double quotes.

"""

"""Set whether the command line invocation of this code should be escaped with double quotes.

:param use_double_quotes: boolean, True if to escape with double quotes, False otherwise.

"""

This was resolved, but I don't see the changes?

sphuber · 2022-04-08T17:51:07Z

aiida/orm/nodes/data/code.py

+    def get_use_double_quotes(self):
+        """
+        Return whether a code is escape with double quotes (False, default) or not (True).
+        """


"""Return whether the command line invocation of this code should be escaped with double quotes. :returns: boolean, True if to escape with double quotes, False otherwise which is also the default. """

Same here. Don't see the changes.

aiida/cmdline/params/options/commands/code.py

sphuber · 2022-04-08T18:03:37Z

aiida/cmdline/params/options/commands/computer.py

+    is_eager=False,
+    default=False,
+    cls=InteractiveOption,
+    prompt='Whether use double quotes for prepend cmdline params?',


Suggested change

prompt='Whether use double quotes for prepend cmdline params?',

prompt='Escape CLI arguments in double quotes',

aiida/cmdline/params/options/commands/code.py

aiida/cmdline/params/options/commands/computer.py

unkcpz

@sphuber thanks for the review and sorry for the delay of changes and reply, suffer from COVID last week.

I implement most of your suggestions and also docstring of use_double_quotes of JobTemplateCodeInfo.

but one fundamental question: is the separate control of quoting in the Code and Computer necessary?

I think there are two reasons to apply it independently: 1. it is more flexible and more possible for backward compatibility if it can be controlled separately. 2. with initial containerized code implementation (the PR after this PR) the specific suffix for using containerized code is independent of cmdline_params parts of code and double quotes escape needed since I want to use $PWD to map working directory to the directory where the inputs locate. While for the code part it can keep the same.

aiida/manage/tests/pytest_fixtures.py

unkcpz · 2022-04-19T15:23:18Z

aiida/orm/computers.py

+    def get_use_double_quotes(self) -> bool:
+        return self.get_property('use_double_quotes', False)
+
+    def set_use_double_quotes(self, val: bool) -> None:
+        self.set_property('use_double_quotes', bool(val))


Thanks, changed.

unkcpz · 2022-04-19T15:28:02Z

aiida/schedulers/datastructures.py

    cmdline_params: list[str] = field(default_factory=list)
+    use_double_quotes: list[bool] = field(default_factory=list)


prepend_cmdine_params is to collect all computer related params (mpi parameters, scheduler parameters) that are shown in front of the executable and arguments of code.

unkcpz · 2022-04-19T15:35:49Z

tests/engine/processes/calcjobs/test_calc_job/test_computer_double_quotes_True_.sh

+exec 2> _scheduler-stderr.txt
+
+
+"mpirun" "-np" "1" '/bin/bash' < 'aiida.in' > 'aiida.out' 2> 'aiida.err'


Yes, I think so. I didn't implement it since it is not relevant to the following up features. It makes sense to also call these the computer related parameters. Changed.

unkcpz · 2022-04-19T15:54:18Z

tests/engine/processes/calcjobs/test_calc_job.py

+        codeinfo.stderr_name = 'aiida.err'
+
+        calcinfo = CalcInfo()
+        calcinfo.codes_info = [codeinfo]


Yes, it is affected. test added.

unkcpz · 2022-04-19T15:59:30Z

tests/cmdline/commands/test_code.py

+        '--on-computer',
+        f'--computer={aiida_localhost.label}',
+        '--remote-abs-path=/remote/abs/path',
+        '--use-double-quotes',


It was added to check if this option can be used and take effect. But rather not test it here. Removed.

sphuber · 2022-04-20T10:04:34Z

Thanks @unkcpz . Hope you are feeling better. I noticed that you resolved a few comments that don't seem to actually have been addressed. At least I cannot see the changes of the suggestions I made. Also the tests and pre-commit are failing. Please fix those and update the branch.

unkcpz · 2022-04-20T12:37:18Z

@sphuber sorry for the carelessness, I wrongly mix up commits and push to the actual container code implementation branch locally previously. Now it is rebased a bit and passes all the tests.

So far, the command line arguments of the code exeuction line in the submit script written by the `Scheduler` plugin, would be wrapped by single quotes to prevent special characters, such as spaces, from being interpreted incorrectly. However, for certain use cases, the quoting has an undesirable effect. For example, if an argument contains a bash variable, indicated by a `$` sign, the single quotes will prevent it from being dereferenced correctly. A concrete use-case is the requested feature of being able to define a Docker container as a `Code`, which needs certain arguments to be quoted in double quotes. This PR adds a new property `use_double_quotes` to the `Code` and `Computer` classes that determines whether arguments should be quoted using single or double quotes. The settings are communicated through the `cmdline_params` attribute of a `JobTemplateCodeInfo` instance to the scheduler plugin. The plugin is then responsible for respecting these booleans. To be able to distinguish between the command line arguments of the computer and the code, they are passed separately in `prepend_cmdline_params` and `cmdline_params`, respectively.

sphuber · 2022-04-22T09:43:57Z

Thanks @unkcpz !

unkcpz requested review from ltalirz and sphuber April 8, 2022 11:07

sphuber requested changes Apr 8, 2022

View reviewed changes

unkcpz commented Apr 19, 2022

View reviewed changes

unkcpz force-pushed the design/double-quote-for-computer-code branch from 24eabaf to 964ba11 Compare April 19, 2022 16:17

unkcpz requested a review from sphuber April 19, 2022 16:18

unkcpz force-pushed the design/double-quote-for-computer-code branch 4 times, most recently from 1797d75 to 4a21ea5 Compare April 20, 2022 12:15

sphuber force-pushed the design/double-quote-for-computer-code branch from 4be3c8b to 38b14dc Compare April 22, 2022 09:16

sphuber approved these changes Apr 22, 2022

View reviewed changes

sphuber merged commit 84c67ba into aiidateam:develop Apr 22, 2022

unkcpz deleted the design/double-quote-for-computer-code branch April 22, 2022 09:49

unkcpz mentioned this pull request Sep 27, 2022

containerized code only for remote mpi mapping hpc type #5667

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Containerized code: double quotes setting of computer and code setup #5478

Containerized code: double quotes setting of computer and code setup #5478

unkcpz commented Apr 8, 2022

ltalirz commented Apr 8, 2022 •

edited

Loading

unkcpz commented Apr 8, 2022

sphuber left a comment

sphuber Apr 8, 2022

unkcpz Apr 19, 2022

sphuber Apr 8, 2022

sphuber Apr 20, 2022

sphuber Apr 8, 2022

sphuber Apr 20, 2022

sphuber Apr 8, 2022

unkcpz left a comment •

edited

Loading

unkcpz Apr 19, 2022

unkcpz Apr 19, 2022

unkcpz Apr 19, 2022

unkcpz Apr 19, 2022

unkcpz Apr 19, 2022

sphuber commented Apr 20, 2022

unkcpz commented Apr 20, 2022

sphuber commented Apr 22, 2022

-        """
-        Set whether a code cmdline is escape with double quotes.
-        """
+        """Set whether the command line invocation of this code should be escaped with double quotes.
+        :param use_double_quotes: boolean, True if to escape with double quotes, False otherwise.
+        """

	prompt='Whether use double quotes for prepend cmdline params?',
	prompt='Escape CLI arguments in double quotes',

		cmdline_params: list[str] = field(default_factory=list)
		use_double_quotes: list[bool] = field(default_factory=list)

		exec 2> _scheduler-stderr.txt


		"mpirun" "-np" "1" '/bin/bash' < 'aiida.in' > 'aiida.out' 2> 'aiida.err'

Containerized code: double quotes setting of computer and code setup #5478

Containerized code: double quotes setting of computer and code setup #5478

Conversation

unkcpz commented Apr 8, 2022

ltalirz commented Apr 8, 2022 • edited Loading

unkcpz commented Apr 8, 2022

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber commented Apr 20, 2022

unkcpz commented Apr 20, 2022

sphuber commented Apr 22, 2022

ltalirz commented Apr 8, 2022 •

edited

Loading

unkcpz left a comment •

edited

Loading