Add node_value & get_node_value to TaskSocket

[GeigerJ2]

This is basically just syntactic sugar to directly access the associated Python variable of a Socket, avoiding .value.value constructs.

Fixes #293.

A few notes regarding the implementation:

• I'm checking first the nested case, as the non-nested one should always match, and, in the case of an AiiDA ORM type, return the AiiDA ORM variable, not its value
• I implemented it in TaskProperty not SocketAny, as all sockets inherit from TaskProperty but not SocketAny
• In the future, when we (re-)introduce the SocketAiiDADict, we can overwrite the method in this specific class, using get_dict rather than .value.value
• Do we even need to provide a property? We can just provide the get_node_value method? The property is useful for caching, though.
• One could use try-except to avoid writing the attribute that is being checked as string which can be a pain for later refactoring. However, for now I think the hasattr call reads cleaner
• In the parametrization of the tests, passing AiiDA ORM types would require loading a profile (passing the aiida_profile fixture doesn't work), so I'm just type-hinting AiiDA ORM types, but passing the Python types, which still creates the correct sockets, but maybe there's a better solution?

[superstar54]

Hi @GeigerJ2 , thanks for the work!

I implemented it in TaskProperty not SocketAny

I think you mean TaskSocket.

Do we even need to provide a property?

I think we need it, because it's convenient. socket.node_value is short compared to socket.get_node_value().

maybe there's a better solution?

I don't have a solution either 😢 . However, since the value will be converted to an orm.Data object inside the socket, the test successfully achieves its intended goal.

I also have a few comments on the code:

• _node_value is not needed, because we only need to read the raw value, not modify it. There is also no need for caching, because it's not a heavy calculaiton.
• I understand that you intend to handle as many socket types within the get_node_value method, but this introduces many logic (if..else) into the method. I would prefer keepting the get_node_value method in the TaskSocket unimplemented, and raising NotImplemented if the user attempt to use it. Then implement the specific method in each Socket type. For the moment, you can implement the one for the SocketAny.

[GeigerJ2]

Hi @GeigerJ2 , thanks for the work!

Cheers!

I implemented it in TaskProperty not SocketAny

I think you mean TaskSocket.

That was indeed a spelling mistake, my bad!

Do we even need to provide a property?

I think we need it, because it's convenient. socket.node_value is short compared to socket.get_node_value().

Yeah, apart from the shorter name, the idea was also to store it in the private _node_value, so node_value doesn't have to call get_node_value() every time. Though, one can just assign that to a variable when calling it anyway, and it's not a heavy calculation, as you say below, so fine for me!

maybe there's a better solution?

I don't have a solution either 😢 . However, since the value will be converted to an orm.Data object inside the socket, the test successfully achieves its intended goal.

That's not possible when only implemented in SocketAny, see below.

I also have a few comments on the code:

• _node_value is not needed, because we only need to read the raw value, not modify it. There is also no need for caching, because it's not a heavy calculaiton.

OK!

• I understand that you intend to handle as many socket types within the get_node_value method, but this introduces many logic (if..else) into the method. I would prefer keepting the get_node_value method in the TaskSocket unimplemented, and raising NotImplemented if the user attempt to use it. Then implement the specific method in each Socket type. For the moment, you can implement the one for the SocketAny.

The thing is, if implementing only in SocketAny for now, the testing I wrote becomes a bit useless, as providing any type hinting will give the specific Socket(AiiDA)[Int|Float|Str|Bool], rather than SocketAny. Then, in addition, as one cannot pass instantiated AiiDA ORM types, neither type hints as just mentioned, we cannot test at all for any SocketAny that has an AiiDA ORM attached.

In my last commit, the implementation is as you propose. For me, that's fine to get merged.

Locally, I have another version where I define a SocketBase intermediate class that inherits from TaskSocket and implements the get_node_value method. Then the other sockets, SocketAny, and the more specific Socket(AiiDA)[Int|Float|Str|Bool] inherit from that, rather than TaskSocket:

from aiida import orm


class SocketBase(TaskSocket):
    def get_node_value(self):
        "Obtain the value of the associated data of a `Socket`."

        if isinstance(self.value, orm.Data):
            if hasattr(self.value, "value"):
                return self.value.value
            else:
                raise ValueError(
                    "Data node does not have a value attribute. We do not know how to extract the raw Python value."
                )
        else:
            return self.value


class SocketAny(SocketBase, SerializePickle):
    """Any socket."""
	...
  
class SocketFloat(SocketBase, SerializeJson):
    """Float socket."""
	...

class SocketAiiDAFloat(SocketBase, SerializeJson):
    """AiiDAFloat socket."""
	...

Though, not sure if that's useful (beyond the current case) or even desired design, so I didn't commit it.

[superstar54]

The thing is, if implementing only in SocketAny for now, the testing I wrote becomes a bit useless ...
Locally, I have another version where I define a SocketBase intermediate class

I see. Let's make it simple by moving the get_node_value method from SocketAny to the TaskSocket.

[GeigerJ2]

I see. Let's make it simple by moving the get_node_value method from SocketAny to the TaskSocket.

My saying :D OK, I reverted the latest commit, and updated the implementation. Now it should be ready to merge. Could you please give it a final review?

[superstar54]

LGTM!