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

[OM] Simplify the Python instantiate API to just return Objects. #5400

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

mikeurbach
Copy link
Contributor

This API previously accepted a Python dataclass as input, and used it as a template to guide the logic to pull fields out of the instantiated Object and build an instance of the requested dataclass.

This allowed the API to be strongly typed, by accepting a dataclass type as input and returning an instance of that dataclass as output. However, this requires the user to specify a-priori what fields should be present in the resulting Object, and this may not always be known.

By simply returning an Object, and adding the appropriate Python conversions to Object's getattr, we can generically return Objects that behave just like the previous dataclasses, without specifying the fields up front. This also avoids rewrapping the Objects in dataclasses.

In the future, we can add back a similar form of type safety when this would be useful, potentially using Protocols similarly to how dataclasses were used as a template before.

This API previously accepted a Python dataclass as input, and used it
as a template to guide the logic to pull fields out of the
instantiated Object and build an instance of the requested dataclass.

This allowed the API to be strongly typed, by accepting a dataclass
type as input and returning an instance of that dataclass as
output. However, this requires the user to specify a-priori what
fields should be present in the resulting Object, and this may not
always be known.

By simply returning an Object, and adding the appropriate Python
conversions to Object's __getattr__, we can generically return Objects
that behave just like the previous dataclasses, without specifying the
fields up front. This also avoids rewrapping the Objects in
dataclasses.

In the future, we can add back a similar form of type safety when this
would be useful, potentially using Protocols similarly to how
dataclasses were used as a template before.
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

Hmm, that's unfortunate that we lose static typing but it totally makes sense to me. Maybe we can make instantiate to give static type only when dataclass type is given, something like instantiate(self, cls: Union[str, type["DataclassInstance"]], *args: Any) -> Union[Object, "DataclassInstance"]. Not sure mypy is smart enough to perform this kind of flow sensitive typing, but anyway the current implementation looks simple and great to me!

Copy link
Contributor

@prithayan prithayan left a comment

Choose a reason for hiding this comment

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

This solution looks great, I echo Hideto's comments, would be great to include the DataClassInstance option in the future.

@mikeurbach
Copy link
Contributor Author

Thanks, yeah I think we can do something interesting here. I think for now, I'd prefer to do something simple that works, and figure out the static typing story once we're happy with the actual functionality.

@mikeurbach mikeurbach merged commit c5aff9a into main Jun 14, 2023
@mikeurbach mikeurbach deleted the mikeurbach/om-dialect-python-api-objects branch June 14, 2023 15:44
@mikeurbach
Copy link
Contributor Author

Thanks for the reviews. I think what we can probably do is add a method that takes a protocol and verifies that an object implements the protocol. That's basically a simple type checker. We should be able to compose the API I just merged with that new API, and have something like instantiate_typed which makes the objects, type checks them, and has a signature that convinces mypy et al that the object is safe.

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.

3 participants