Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameterized domain types --- a mechanism for sub-workflow templates #4

Closed
wants to merge 7 commits into from

Conversation

SimonHeybrock
Copy link
Member

@SimonHeybrock SimonHeybrock commented Jul 10, 2023

Initial attempts at making use of child-containers (child injectors) for support sub-workflows proved challenging: I ran into both technical issues (injector did not behave as I had assumed when working with child injectors) as well as UX ones (the client code was complex to see through, even for me who came up with it).

Therefore, I moved to a more explicit and "pedestrian" approach. It should be easier to see through.

The main challenge here was to find a way of defining "generic" domain types. We want to use NewType to, e.g., turn a scipp.DataArray into something specific, but would like to use a syntax such as IofQ[Sample] to refer to the IofQ of the "Sample" run. parametrized_domain_type is the current result of this.

Copy link
Member

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising!

"""
Return a type-factory for parametrized domain types.

The types return by the factory are created using typing.NewType. The returned
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The types return by the factory are created using typing.NewType. The returned
The types returned by the factory are created using typing.NewType. The returned

"""

class Factory:
_subtypes: Dict[str, type] = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So parametrized_domain_type must only be called once per name? That is, does using Monitor = parametrized_domain_type("Monitor", DataArray) in two different files lead to distinct types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell this is the same for typing.NewType?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. I just wanted to make sure. This looks like the right thing to do.


def reduction_factory(tp: type) -> List[Callable]:
def incident_monitor(x: Raw[tp]) -> IncidentMonitor[tp]:
return IncidentMonitor[tp](x.monitor1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could avoid repeating the return type. The injector should be able to convert the return value to the correct type. (Would likely need modifications to injector package.) But would this mess up type hinting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least for run-time, you can just omit it. I don't know if the type-checker needs it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried it and neither mypy nor pyre like it when I just return the base type. So I guess we need to explicitly wrap the result.

Comment on lines +36 to +37
def reduction_factory(tp: type) -> List[Callable]:
def incident_monitor(x: Raw[tp]) -> IncidentMonitor[tp]:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy does not like this: tp is a variable (holding a type) and cannot be used for type-checking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could try

def reduction_factory(tp: type) -> List[Callable]:
    T = TypeVar("T", bound=tp)

    def incident_monitor(x: Raw[T]) -> IncidentMonitor[T]:

But this probably doesn't work either.

@SimonHeybrock
Copy link
Member Author

I think python/mypy#3331 might help to resolve these problems, but it has seen little progress over time.

@SimonHeybrock
Copy link
Member Author

Closing, in favor of a slightly different approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants