Fix prepare_shell_job_inputs to be compatible with PortableCode

[t-reents]

A PortableCode is not associated with a computer, so the one specified through the metadata needs to be passed back so it reaches the ShellJob and is set on the CalcJobNode directly.

This is related to an issue reported on AiiDA Discourse: https://aiida.discourse.group/t/from-bash-to-aiida/752

[t-reents]

@GeigerJ2 pinging you here, since I can't request a review

[giovannipizzi]

Are we essentially popping it, and then putting it back?

Anyway if you tested it, I'm OK (since it might have been put in the wrong place). Maybe a comment that you are putting back in the metadata, even when it was in metadata options (therefore the popping and reinsert), might be useful (and explaining instead that for InstalledCodes the metadata.computer is not set and mentioning why)

[t-reents]

Thanks @giovannipizzi !

Indeed, and I was also thinking if it could be simplified (I just thought that there might be a reason against). After some further checks, I think that the change can be as easy as just replacing pop with get, as done in the latest commit. This avoids the nested logic.

The idea is that in case of a "string code", the code is anyway associated with the specified computer, see prepare_code. The same is true for a portable code. The only scenario that I could think of where it could cause problems is when an InstalledCode is specified and a different computer (than the one associated with the code) is specified in the metadata. However, in that case the validation of the CalcJob in aiida-core will simply raise, because it checks that the specified computer and the one of the code agree.

The new version still fixes the issue and the tests also pass. Unfortunately, a new approval is required.

[giovannipizzi]

I cannot test but it seems ok. Approving but great to test explicitly that it works for the various cases (ideally is it possible to add a simple test?)

[t-reents]

I adapted an existing test to test several code types (I also checked that the test actually fails without the fix). Moreover, since a new review was anyway required, I already squashed the commits in a single commit so that it can be easily merged.

[t-reents]

@GeigerJ2 Could I request your review and ask you to take care of merging? :) Since Giovanni is on vacation etc.

[GeigerJ2]

LGTM, just posted one minor nit, as I don't like variable re-assignment, but not a blocker. Thanks, @t-reents!

[GeigerJ2]

One variable fed by options then conditionally re-fed from the top level, so the if/else reads as two ways to obtain a computer. What happens is simpler: a legacy spelling gets promoted, then one lookup resolves it. if computer: is also a truthiness test on an Optional, where line 220 already uses is not None. Suggestion for L126-138:

    metadata = metadata or {}
    options = metadata.get('options', {})

    if 'computer' in options:
        warnings.warn(
            'Specifying a computer through `metadata.options.computer` in `launch_shell_job` is deprecated. Please use '
            '`metadata.computer` instead.',
            AiidaDeprecationWarning,
            stacklevel=2,
        )
        metadata['computer'] = options.pop('computer')

    computer = metadata.get('computer')

Same mutation semantics, drop-in. Verified as written: 8/8 local cases (the four parametrisations, deprecated path for string and portable, no-computer-anywhere localhost fallback, degenerate metadata shapes None/{}/{'options': {}}), mypy clean, ruff@0.8.6 (the pinned version) All checks passed! and already formatted.

Two tidy-ups if you take it: metadata.get('computer', None) → metadata.get('computer'), and launch.py:158 'metadata': metadata or {} is redundant after line 126.

Separate decision: prepare_shell_job_inputs mutates the caller's metadata (the pop predates this, the write is new), so a caller reusing one dict sees the deprecation warning only on the first launch. Copying both levels fixes it and also passes, but changes behaviour for anyone relying on the in-place edit.

[GeigerJ2]

Thanks, @t-reents, LGTM!