Double quote escape for bash script #5280

unkcpz · 2021-12-17T13:01:02Z

I separate the implementation out from the container code PR. Also slightly touch and trying to address the issue of environment_variables of calcjob mentioned at #4836.

I make a demo change on SGE scheduler and found a bug there (please see the second commit 9d2f639e6). If you think this is the right direction for #4836. I will continue on implement this for other schedulers.

pinning @bosonie for comment since you open the issue mentioned @giovannipizzi to double-check this is the right way to do the escape. Also @mbercx since maybe you need more magic about escape in you hyperqueue plugin?

sphuber

I am fine with the principle of adding a new option that can change the default behavior. Just have a question about the name and the fact that each subclass explicitly has to be updated to respect the new option. I think this requires a change in the setup of Scheduler._get_submit_script_header.

sphuber · 2021-12-17T14:10:46Z

aiida/engine/processes/calcjobs/calcjob.py

@@ -244,6 +244,8 @@ def define(cls, spec: CalcJobProcessSpec) -> None:  # type: ignore[override]
            help='If set to true, the submission script will load the system environment variables',)
        spec.input('metadata.options.environment_variables', valid_type=dict, default=lambda: {},
            help='Set a dictionary of custom environment variables for this calculation',)
+        spec.input('metadata.options.use_double_quotes', valid_type=bool, default=False,


Since this only applies to the environment_variables option, should we name this option more explicitly, like environment_variables_double_quotes? This makes it a lot clearer that it only affects that.

Maybe envvar_double_quotes instead of environment_variables to avoid to have a super-long name that users have to type? I'm not 100% sure of this name though

I agree it is not precise to naming it as use_double_quotes but environment_variables_double_quotes really too long. My initial consideration is that use_double_quotes also influent other parts like command line parameters. Do you think it is too much to do this in the calcjob metadata setting with one option?

aiida/schedulers/plugins/sge.py

giovannipizzi

The thing that is missing is how to use double quotes for the computer and code properties. E.g. the mpirun command (this is what we need for the hyperqueue plugin). Pinging also @mbercx.

Would a simple, backward-compatible option be to add a _USE_DOUBLE_QUOTES class variable for scheduler plugins? And AiiDA checks if it exists and honours it if present (going back to single quotes if it's not present of if the value is False).

Or even, if we want to give more flexibility, we could have _QUOTE_MODE variable that accepts a value from an Enum, e.g. SINGLE (default), DOUBLE (new option), or even NONE. @sphuber what do you think?

giovannipizzi · 2021-12-17T14:26:12Z

aiida/engine/processes/calcjobs/calcjob.py

@@ -244,6 +244,8 @@ def define(cls, spec: CalcJobProcessSpec) -> None:  # type: ignore[override]
            help='If set to true, the submission script will load the system environment variables',)
        spec.input('metadata.options.environment_variables', valid_type=dict, default=lambda: {},
            help='Set a dictionary of custom environment variables for this calculation',)
+        spec.input('metadata.options.use_double_quotes', valid_type=bool, default=False,


Maybe envvar_double_quotes instead of environment_variables to avoid to have a super-long name that users have to type? I'm not 100% sure of this name though

sphuber · 2021-12-17T14:43:05Z

The thing that is missing is how to use double quotes for the computer and code properties.

If the point of this is really to affect quoting in general, then having an option called quote_style (that can have a number of options as you said) would probably be better yeah. Big question remains though how we can make sure that plugins respect this. As I indicated in another comment, with the current design, the option would probably not be respected for the environment variables since each plugin has to reimplement this. Existing ones outside of aiida-core will not do this clearly, after this change.

Anyway, I opened PR #5283 to abstract the environment variable part to solve that, but if we extend the quoting also to other things of the script, such as the mpi_command, then I am not sure if there is an easy way for us to guarantee this is respected by plugins.

sphuber · 2021-12-17T14:43:53Z

And just for my understanding, is @unkcpz implementing this just because of @bosonie feature request, or is this needed elsewhere as well? Would be good to have those specific use case requirements as well if that is the case.

unkcpz · 2021-12-17T14:47:52Z

And just for my understanding, is @unkcpz implementing this just because of @bosonie feature request, or is this needed elsewhere as well?

I implement this in order to affect the quotes style in general, just thought @bosonie feature request is another case that can be addressed by extra parameter of scape_for_bash and fixed first.

giovannipizzi · 2021-12-17T14:59:18Z

It's needed at least for the aiida-hyperqueue plugin (see aiidateam/aiida-hyperqueue#2).
Also, the PR of @unkcpz on supporting containerised code seemed to need this, right (to specify mount point etc.)

unkcpz · 2021-12-20T10:59:22Z

@sphuber Thanks for the updating! I rebased this pr and make the changes.

codecov · 2021-12-20T11:20:58Z

Codecov Report

Merging #5280 (0b2db9d) into develop (dce0041) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #5280      +/-   ##
===========================================
+ Coverage    82.13%   82.14%   +0.02%     
===========================================
  Files          533      533              
  Lines        38485    38515      +30     
===========================================
+ Hits         31605    31635      +30     
  Misses        6880     6880

Flag	Coverage Δ
django	`77.23% <100.00%> (+0.02%)`	⬆️
sqlalchemy	`76.53% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/common/datastructures.py	`100.00% <ø> (ø)`
aiida/schedulers/datastructures.py	`94.55% <ø> (ø)`
aiida/cmdline/commands/cmd_code.py	`90.58% <100.00%> (+0.04%)`	⬆️
aiida/cmdline/params/options/commands/code.py	`100.00% <100.00%> (ø)`
aiida/common/escaping.py	`100.00% <100.00%> (ø)`
aiida/engine/processes/calcjobs/calcjob.py	`90.32% <100.00%> (+0.12%)`	⬆️
aiida/manage/tests/pytest_fixtures.py	`94.32% <100.00%> (+0.07%)`	⬆️
aiida/orm/computers.py	`82.42% <100.00%> (+0.24%)`	⬆️
aiida/orm/nodes/data/code.py	`91.86% <100.00%> (+0.23%)`	⬆️
aiida/orm/utils/builders/code.py	`85.13% <100.00%> (+0.26%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dce0041...0b2db9d. Read the comment docs.

sphuber

Thanks @unkcpz . Some changes to the docstrings, but the rest of the implementation is ok. I have to admit that I am still not a fan of envvar_double_quotes. It may very well be a bit shorter, but I tend to think that shorter is not better if you are sacrificing consistency and clarity. But if others think this is the best option then I won't stop it.

aiida/common/escaping.py

aiida/engine/processes/calcjobs/calcjob.py

aiida/schedulers/datastructures.py

unkcpz · 2021-12-20T14:24:44Z

@sphuber Thanks, I make the changes as per your advice.

For envvar_double_quotes I personally think this is name is clear and concise, although this is not the pythonic naming style for sure.

giovannipizzi · 2021-12-21T09:22:48Z

Mmm, I see the point on the naming - I realise that there is also an environment_variables option, so indeed it's not very consistent. However very long names are also a bit annoying...

So I see two options (for the variable indicating whether to escape with double quotes (instead of single quotes) the value of the environment variables:

envvar_double_quotes
environment_variables_double_quotes

Maybe someone else who'd be actually using this feature might be interested in chipping in? E.g. @bosonie @mbercx

More generally - I see this PR is only about env vars. What about telling to use double quotes e.g. for the command line arguments of a code (will this be a new attribute in the CalcInfo returned by the plugin?), or to the mpirun_command of the computer (will this be an option in the computer setup?). I imagine this is the (partial) focus of #5250 - just asking to make sure we don't want this option defined in this PR also control escaping of other parts of the script (thinking to it, I think this is OK as it is, and the other properties are not specific to CalcJob but to either Code, CalcInfo or Computer).

unkcpz · 2022-01-07T12:44:45Z

I add an extra commit 8d3463d to this PR to enable double-quotes setting for calcjob job script code command. This option is a computer property and can be set by computer.set_use_double_quotes(True). I think it will work for both @mbercx hyperquene case and my container code to have ENV in the code executable command of the job script.

To summary, the main dilemma in adding this double-quotes setting is where this option should be set, if it is set in multiple places below what is the order of override?

Set double-quotes as a computer property. (in the new commit, only this is applied. There is no problem for hyperqueue case since one computer has only one scheduler and hyperqueue implement a new scheduler and it is bound to the computer.)
Set it along with the code set up. (With this we can only use double quotes for container code, it is more flexible and backward compatible, but it is complex do decide whether this should override the double quotes setting above).
Allowed to set it in CalcJob object option. (Have the same issue in how to override the option set for computer, but this will give flexibility for CalcJob users to tune this parameter as they expect.)

unkcpz · 2022-01-25T23:35:24Z

The way of command execute line in script generating from code_info and code is now shaped to:

computer_cmdline_params[0] computer_cmdline_params[1] ... cmdline_params[0] cmdline_params[1] ... < stdin > stdout

add computer_cmdline_params for CodeInfo which separate the parameters such as mpirun and scheduler related (in aiida-hyperqueue case) from cmdline_params.
add use_double_quotes which if True the cmdline_params will all escape by double-quotes.
add custom_cmdline_string, if this string is set, the whole executable line will not be escaped but left as it is. This will be the solution for the tricky command line of executable code. such as the docker run and keep the maximum flexibility to configure the line.

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

for more information, see https://pre-commit.ci

unkcpz · 2022-02-11T14:59:18Z

Close this PR since it is included in the containerized PR and will be separated into different small PRs include #5349

sphuber self-requested a review December 17, 2021 14:09

sphuber reviewed Dec 17, 2021

View reviewed changes

giovannipizzi reviewed Dec 17, 2021

View reviewed changes

unkcpz force-pushed the double-quote-bash branch 3 times, most recently from e0b9b13 to f85cac4 Compare December 20, 2021 10:57

unkcpz requested a review from sphuber December 20, 2021 10:58

sphuber requested changes Dec 20, 2021

View reviewed changes

aiida/common/escaping.py Show resolved Hide resolved

aiida/engine/processes/calcjobs/calcjob.py Outdated Show resolved Hide resolved

aiida/schedulers/datastructures.py Outdated Show resolved Hide resolved

unkcpz force-pushed the double-quote-bash branch 5 times, most recently from 3c800d6 to 4758dbb Compare January 25, 2022 21:25

unkcpz requested review from sphuber and giovannipizzi January 25, 2022 23:35

unkcpz force-pushed the double-quote-bash branch from 58493be to 4adcef8 Compare February 8, 2022 21:56

unkcpz added 4 commits February 8, 2022 23:29

escape bash with double quotes as an optional

ccc5329

address escape issue for ENV variable

5816956

review

e61fd73

double quotes set from computer and to exec command line

56fd860

unkcpz added 11 commits February 8, 2022 23:29

update test

034e77b

seperate std input output

54ad568

computer_cmdline_params

77e996d

add use_double_escape to Code and CodeInfo

2e968b4

unittest

a8c5ce1

add computer_cmdline_params to code_info

2f5b53f

Add custom string for code info to maximum flexibility

02067c3

rename to prepend_cmdline_params

549f43f

code_info use_double_quotes as a tuple

e1e758d

explicitly set use_double_quotes in every scheduler test

9e23623

code setup

ba40f5a

unkcpz force-pushed the double-quote-bash branch from 3b212fd to ba40f5a Compare February 8, 2022 22:59

[pre-commit.ci] auto fixes from pre-commit.com hooks

0b2db9d

for more information, see https://pre-commit.ci

unkcpz mentioned this pull request Feb 11, 2022

Double quote escape for bash script ENV_VAR part #5349

Merged

unkcpz closed this Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Double quote escape for bash script #5280

Double quote escape for bash script #5280

unkcpz commented Dec 17, 2021

sphuber left a comment

sphuber Dec 17, 2021

giovannipizzi Dec 17, 2021

unkcpz Dec 17, 2021

giovannipizzi left a comment

giovannipizzi Dec 17, 2021

sphuber commented Dec 17, 2021

sphuber commented Dec 17, 2021

unkcpz commented Dec 17, 2021

giovannipizzi commented Dec 17, 2021

unkcpz commented Dec 20, 2021

codecov bot commented Dec 20, 2021 •

edited

Loading

sphuber left a comment

unkcpz commented Dec 20, 2021

giovannipizzi commented Dec 21, 2021

unkcpz commented Jan 7, 2022

unkcpz commented Jan 25, 2022

unkcpz commented Feb 11, 2022

Double quote escape for bash script #5280

Double quote escape for bash script #5280

Conversation

unkcpz commented Dec 17, 2021

sphuber left a comment

Choose a reason for hiding this comment

sphuber Dec 17, 2021

Choose a reason for hiding this comment

giovannipizzi Dec 17, 2021

Choose a reason for hiding this comment

unkcpz Dec 17, 2021

Choose a reason for hiding this comment

giovannipizzi left a comment

Choose a reason for hiding this comment

giovannipizzi Dec 17, 2021

Choose a reason for hiding this comment

sphuber commented Dec 17, 2021

sphuber commented Dec 17, 2021

unkcpz commented Dec 17, 2021

giovannipizzi commented Dec 17, 2021

unkcpz commented Dec 20, 2021

codecov bot commented Dec 20, 2021 • edited Loading

Codecov Report

sphuber left a comment

Choose a reason for hiding this comment

unkcpz commented Dec 20, 2021

giovannipizzi commented Dec 21, 2021

unkcpz commented Jan 7, 2022

unkcpz commented Jan 25, 2022

unkcpz commented Feb 11, 2022

codecov bot commented Dec 20, 2021 •

edited

Loading