🐛 Protocols: fix silently dropped overrides in get_builder_from_protocol

[t-reents]

get_builder_from_protocol() built each work chain's inputs by manually copying a hardcoded whitelist of fields onto the builder, wrapping each in its AiiDA type by hand. Any overrides key not in that whitelist (e.g. monitors) was silently ignored instead of being applied or raising an error.

• Replace the manual per-field copying with builder._update(inputs) in PwBaseWorkChain, PhBaseWorkChain, PdosWorkChain, PwBandsWorkChain, PwRelaxWorkChain and NebBaseWorkChain. This existing ProcessBuilderNamespace method already does everything the manual code was doing by hand: it merges a plain dict onto the builder, applies the default to_aiida_type serializer per port, and raises AttributeError for unknown keys at a non-dynamic level.
• A typo'd or misplaced key at a work chain's root namespace now raises instead of being dropped. Keys nested inside a dynamic namespace (e.g. pw, base_relax) still can only be caught with the existing UserWarning from _validate_override_keys, since dynamic namespaces accept arbitrary keys by design — this soft check is unchanged and keeps working exactly as introduced in 👌 Protocols: Add validation on overrides keys #1136.
• While this changes the behavior, i.e., failing for top level keys, I'd still say that it's fine. I also had a version just emitting a warning and ignoring those keys, but I found it unnecessarily complicated. In the end, an error is only raised for top level keys and if a user tries to manually set them on the builder, it will anyway fail. Also, in 🐛 get_builder_from_protocol: Fix bugs when overriding pseudopotentials #1102 we introduced a new raising behavior that was included in a minor release, so I think that we could accept this change and properly document it in the CHANGELOG.

If there are other opinions, especially in favour of checking those top level keys and dropping them with a warning, instead of raising, we can of course adjust it

[t-reents]

@elinscott I'd suggest that we first agree on this and get it merged and afterwards, we can add the tests to your PR #1274 that validate that all possible overrides according to your TypedDictss actually get merged into the builder.

[elinscott]

Thanks, looks good by and large, and the tests are discriminating — I reverted pw/base.py and pw/relax.py against them and they fail.

One blocking issue and some smaller things listed in comments below.

[elinscott]

_update raises KeyError for any dict-valued key at the root namespace, including valid ones.

ProcessBuilderNamespace._update does self[key].update(value) when the value is a Mapping. But self[key] is self._data[key] which is only pre-populated for namespace ports, which for e.g. PwBaseWorkChain is just metadata and pw, so handler_overrides (a genuine Dict input port) hits KeyError instead of landing:

PwBaseWorkChain.get_builder_from_protocol(
    code, structure,
    overrides={'handler_overrides': {'handle_known_unrecoverable_failure': {'enabled': False}}},
)
# main: builds, value silently dropped
# here: KeyError: 'handler_overrides'

This is also an issue one level down, where the key check finds nothing wrong and so the user gets no warning at all, only the bare KeyError:

PwBandsWorkChain.get_builder_from_protocol(
    code, structure,
    overrides={'scf': {'handler_overrides': {'handle_known_unrecoverable_failure': {'enabled': False}}}},
)

The same mechanism means an invalid dict-valued root key gives KeyError: 'relax' rather than the AttributeError: Unknown builder parameter: relax the PR description promises.

This was not caught because the tests only cover scalar-valued root keys (clean_wokdir: True, pseudo_family: '...'), which is the half that works.

builder._merge(inputs) appears to be a drop-in fix: it routes everything through __setattr__, so unknown keys give the intended AttributeError regardless of value type, and handler_overrides serialises to a Dict as it should. Claude tested the _update → _merge swap and everything seems to work as intended.

[elinscott]

Nested keys in a dynamic namespace land raw.

The description says these are "still only caught with the existing UserWarning", but since pw is dynamic, _update now stores them, and the builder stops validating:

PwBaseWorkChain.get_builder_from_protocol(
    code, structure, overrides={'pw': {'options': {'max_wallclock_seconds': 60}}}, # should be pw.metadata.options.max_wallclock_seconds
)
# main: warning, key absent, spec().inputs.validate() passes
# here: warning, pw.options present, validate() -> "Invalid type <class 'int'> for dynamic port
#       value: expected <class 'aiida.orm.nodes.data.data.Data'>"

This is an ugly way to crash — make the loud failure here is informative.

[elinscott]

Test coverage only tracks two of the six conversions.

None of pdos.py, ph/base.py, pw/bands.py nor neb/base.py are pinned by the tests. The NEB rewrite is the largest of the six and has no protocol test at all, which seems worth at least a smoke-level builder test given it also changes how neb.pw and neb.metadata are assigned.