Scheduler: Move setting of env variables into base class

[sphuber]

The Scheduler base class implements the concrete method _get_submit_script_environment_variables which formats the lines for the submission script that set the environment variables that were defined in the metadata.options.environment_variables input.

However, it was left up to the plugins to actually call this method in the _get_submit_script_header and append the lines to the created submission script header. This would leave it open to plugins forgetting to add this call and essentially have the environment variables specified by the caller of the CalcJob be ignored.

Here we move the call of _get_submit_script_environment_variables to the get_submit_script of the base class, guaranteeing that it will always be called. This still leaves the possibility to the plugin to override the method and customize the behavior.

A check is added just before adding the new call, to check whether the environment variables have already been added indicating the plugin is also still calling the method. In this case a deprecation warning is emitted suggesting that the call can be removed from the plugin.

With this change we do assume that setting the environment variables are idempotent as it is possible that the lines will be included twice until all external plugins have been updated, but this should be a relatively safe assumption.

[ltalirz]

thanks @sphuber , this looks good to me.

Just out of curiosity: what was the problem you ran into (did you write a new scheduler plugin)?
While I agree with the change, given the existing plugins one can copy from I would have imagined it to be rather straightforward to include the call.

The only reason why one would want plugins to do this call explicitly is if there was a need to control the order in which the various scheduler commands are added to the submit script.
I can't think of a case where this would be relevant.

The same thing could probably also be said about the custom_scheduler_commands.

[sphuber]

Just out of curiosity: what was the problem you ran into (did you write a new scheduler plugin)? While I agree with the change, given the existing plugins one can copy from I would have imagined it to be rather straightforward to include the call.

As mentioned in this comment, this came from a discussion on Slack where a user was trying to set an environment variable in the prepend text based on a resource setting: export OMP_NUM_THREADS={num_cpus_per_mpiproc}. They tried by overriding the metadata.options.environment_variables inside the CalcJob plugin, but that is read-only and didn't work. They were seeing different behavior between the direct and torque scheduler. Upon initial look, I thought this may have been due to the torque plugin not calling this _get_submit_script_environment_variables hook, but in fact it does. The difference was actually due to the direct plugin manually setting this export variable as you noticed in #5949 . Anyway, after reading through the code, I thought that even though all the scheduler plugins for aiida-core respect the environment variables, I could easily see this be forgotton by another plugin, which would break the CalcJob interface in essence.

The only reason why one would want plugins to do this call explicitly is if there was a need to control the order in which the various scheduler commands are added to the submit script. I can't think of a case where this would be relevant.

There are some comments on this question in this issue: #3198

The same thing could probably also be said about the custom_scheduler_commands.

I agree. I thought about moving that as well in the PR, but decided against it to not change too much at once. Are there any scheduler commands that could be non-idempotent? If we think they are idempotent, we can also move this to the base class.