process_state is not always there when we probe it

[edan-bainglass]

In NodesTreeWidget._update_tree_node, we call AiiDA's calc_info CLI command, which in turn tries to do process_state = node.process_state.value.capitalize(), which will raise an exception if process_state is None. When using the NodesTreeWidget as part of the ProcessMonitor, I believe the exception will cause the monitor to disable future tree updates.

I recommend a guard in the CLI command for the next AiiDA release. In the meantime, perhaps we bail on the update if there's not yet a process state.

    def _update_tree_node(self, tree_node):
        if isinstance(tree_node, AiidaProcessNodeTreeNode):
            process_node = orm.load_node(tree_node.pk)
            
            # PROPOSED CHANGE
            if not (hasattr(process_node, "process_state") and process_node.process_state):
                return
            # END PROPOSAL
            
            tree_node.name = calc_info(process_node)
            # Override the process state in case that the process node has failed:
            # (This could be refactored with structural pattern matching with py>=3.10.)
            process_state = (
                engine.ProcessState.EXCEPTED
                if process_node.is_failed
                else process_node.process_state
            )
            tree_node.icon_style = self.PROCESS_STATE_STYLE.get(
                process_state, self.PROCESS_STATE_STYLE_DEFAULT
            )

Not sure if we need the hasattr part, but I suppose it doesn't hurt. Also, the check is boolean in general, not specifically is not None, assuming that bailing on empty states is also okay. Correct me if I'm wrong. Though I don't think we can have empty states, as ProcessState is an Enum.

[edan-bainglass]

@danielhollas @AndresOrtegaGuerrero @superstar54 let me know what you think.

[danielhollas]

Sounds reasonable to me, thanks! I'd start with the fix in aiida-core since that might inform the fix in AWB, such as...

Not sure if we need the hasattr part, but I suppose it doesn't hurt.
I'd be surprised if it is needed, I'd hope that process_state is part of the Process ORM and should be there always. Would be nice to avoid hasattr if we can for performance reasons.

Also, the check is boolean in general, not specifically is not None, assuming that bailing on empty states is also okay. Correct me if I'm wrong. Though I don't think we can have empty states, as ProcessState is an Enum.

Sounds reasonable! 👍

Thank you for a very clear write up and for discovering this!

[unkcpz]

Look at the process_state property method of process node

    @property
    def process_state(self) -> Optional[ProcessState]:
        """Return the process state

        :returns: the process state instance of ProcessState enum
        """
        state = self.base.attributes.get(self.PROCESS_STATE_KEY, None)

        if state is None:
            return state

        return ProcessState(state)

The condition can simplified to:

if process_node.process_state is not None:

I guess? Meanwhile, I think the process_state of process_node can be None, see my comment on aiidateam/aiida-core#6682