Refactoring `_get_nodes`

[liamhuber]

I think it would help readability if _to_node_dict_entry were complete and returned a full node-dict entry. This would make it easier to draw a connection to a Function dataclass. By the same token, it appears to specifically mean a function node dict entry, as the composite workflow entries are handled in the previous clause, so a more specific name would help.

Also, the second if hasattr(func, "_semantikon_metadata"): should be redundant -- we can move it's body inside the first clause.

[liamhuber]

Also, the second if hasattr(func, "_semantikon_metadata"): should be redundant -- we can move it's body inside the first clause.

I can't read; the first is "_semantikon_workflow", the second is "_semantikon_metadata". These are not the same words 🤦‍♂

[samwaseda]

Historically, _semantikon_workflow and _semantikon_metadata were attached to the function by simple assignment, i.e.:

def f(x):
    return x

f._semantikon_metadata = some_metadata
f._semantikon_workflow = some_workflow

This had to be changed because mypy was complaining that an ordinary python function does not have these attributes. But except for being two different attributes, they don't have to belong to different classes, so it might be a good idea to have just one to contain both of them.

[liamhuber]

We can independently decorate a function with @u and @workflow, right? I guess that's actually the case that delivers #164 (comment)

I would be fine with any of

• One class with both attributes
• Two separate classes which preserve class mro when one is applied to a function that is already the other
• An explicit diamond structure with a base class, two child classes each have one attribute, and a third child class that is an explicit child of both the independent ones

I think it's just a matter of experimenting a bit and seeing which implementation feels most robust.

At the end of the day, I think it would be nice to replace the if hasattr( tests with if isinstance( tests using the resulting classes.

[liamhuber]

@samwaseda here is a demo of what I meant in #164 (comment), which I don't think is covered by #165:

from semantikon.typing import u
from semantikon.workflow import workflow

def add(x, y):
    z = x + y
    return z

@workflow
@u(some_key="a value")
def double_decorated(a, b):
    n = add(a, b)
    return n

print(type(double_decorated).mro(), hasattr(double_decorated, "_semantikon_metadata"), hasattr(double_decorated, "_semantikon_workflow"))
>>> [<class 'semantikon.workflow.FunctionWithWorkflow'>, <class 'typing.Generic'>, <class 'object'>] True True

@u(some_key="a value")
@workflow
def decorated_double(a, b):
    n = add(a, b)
    return n

print(type(decorated_double).mro(), hasattr(decorated_double, "_semantikon_metadata"), hasattr(decorated_double, "_semantikon_workflow"))
>>> [<class 'semantikon.converter.FunctionWithMetadata'>, <class 'typing.Generic'>, <class 'object'>] True True

Because these classes are actually manually appending attributes (including a run method) to their instances, we safely get all the desired attributes. But we totally lose the structured inheritance -- e.g. mypy can't tell that we will have all the attributes. The inner-decorator class simply gets wiped from the MRO.

As outlined in my previous comment, I think this is soluble in a number of ways. I wanted to actually probe the behaviour anyway, so I thought for posterity I should just post the demo here. I think the demo already hints at how we can write a test to make sure this gets fixed.

[samwaseda]

Yep, it's indeed not covered in my doc string (which it can't be anyway, since the current class doesn't have the two attributes at the same time). I like simple solutions, and one class with two attributes seems the easiest to me. What do you think?

[liamhuber]

Yep, I can live with that. We can make the fields optional dataclasses and have decorator do an isinstance check to see if it should create a new instance or update an existing one.

[liamhuber]

We probably also want to give public properties based on the current private names that are hinted just to the dcs and raise an error if the underlying value is none, so mypy will be happy. Otherwise I think we need two separate classes where the underlying dc is required to be there from instantiation

[samwaseda]

We probably also want to give public properties based on the current private names that are hinted just to the dcs and raise an error if the underlying value is none, so mypy will be happy.

I'm not sure if I understand what you mean by this. If for example only u is applied, then the one to be populated with workflow (i.e. _semantikon_workflow is None, right? Or do you mean something completely different?

[liamhuber]

Yep, that's exactly what I mean. Then after that when we use these dataclasses, we probably want mypy to know for sure that this is what we're getting, so I'm suggesting to add something like

class NoMetadataError(ValueError): ...

@property
def semantikon_metadata(self) -> semantikon.dataclasses.CoreMetadata:
    if self._semantikon_metadata is None:
        raise NoMetadataError(f"The function {self.__name__} has no metadata to access")
    return self._semantikon_metadata

[liamhuber]

Yep, it's indeed not covered in my doc string (which it can't be anyway, since the current class doesn't have the two attributes at the same time). I like simple solutions, and one class with two attributes seems the easiest to me. What do you think?

I want to come back to this and raise a concern: in principle (@)u is the "semantikon" bit, and @workflow is the workflow composition bit. If we want to split these into independent repositories, that will be difficult if we have a single class with two attributes.

One option to keep the idea would be to have the workflow tool depend on semantikon, and to have FunctionWithWorkflow(FunctionWithMetadata) so workflows always have the metadata field even if it's None, but a pure FunctionWithMetadata has no clue about the workflow attribute.

I actually rather like this, but then it strictly locks us into the workflow tool depending on semantikon. (I'm ok with this.)