-
Notifications
You must be signed in to change notification settings - Fork 298
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] Add initial Python API for OM dialect evaluator. #5250
Conversation
lib/Bindings/Python/dialects/om.py
Outdated
for note in diagnostic.notes: | ||
self._logger.info(str(note)) | ||
|
||
# Flush the stderr stream to ensure logs appear when expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stdout?
# Attach our Diagnostic handler. | ||
mod.context.attach_diagnostic_handler(self._handle_diagnostic) | ||
|
||
def instantiate(self, cls: type["DataclassInstance"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type is subscriptable only since python 3.9 PEP-585 so the current implementation will cause import error in python <= 3.8. Perhaps we can use typing.Type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, we should be able to use typing.Type
. I'll also check if from __future__ import annotations
is sufficient, since we have been using that, which is available from 3.7 onwards. I think 3.7 is the oldest we've ever supported, since 3.6 is already EOL and 3.7 will be in the next months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like from __future__ import annotations
works for subscripting type
in 3.7 onward. Since we're already using that pattern, I'll use that here.
lib/Bindings/Python/dialects/om.py
Outdated
# Log the diagnostic message at the appropriate level. | ||
if diagnostic.severity == DiagnosticSeverity.ERROR: | ||
self._logger.error(diagnostic.message) | ||
elif diagnostic.severity == DiagnosticSeverity.ERROR: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo:
elif diagnostic.severity == DiagnosticSeverity.ERROR: | |
elif diagnostic.severity == DiagnosticSeverity.WARNING: |
mod.context.attach_diagnostic_handler(self._handle_diagnostic) | ||
|
||
def instantiate(self, cls: type["DataclassInstance"], | ||
*args: Any) -> "DataclassInstance": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the indented interface of instantiate
? Currently we can pass variadic arguments to instantiate
like evaluator.instantiate(Test, 42, 43, 45)
but it fails while evaluation (specifically at var_to_attribute(*args)
as *args
is unpacked into arguments). I think we have to change the signature into
def instantiate(self, cls: type["DataclassInstance"], args:List[Any])
or to fix the implementation to accept variadic arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I will add a test, my intent was for the variadic API, and to pass the list of values to var_to_attribute
, which will make Attributes out of each element and return an ArrayAttr containing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense!
Awesome! I really love the seamless integration of Python dataclasses and OM classes. One aspect to consider here would be how to represent attribute types in Python that cannot be directly represented, like non-64 bit integers or symbol references. |
Yep, that's a good point. I think I will at least add a code comment about this. My plan for now is to just use the existing circt.support functionality to go to/from attributes. If possible, I'd like to keep enhancing that. But if we find that we need something different, we can define our own library for marshaling back and forth. |
This adds the usual Python API structure and dialect registration boilerplate, as well as Python APIs around the Evaluator library. The C++ implementations with pybind are intended to be as minimal and straightforward as possible, simply wrapping the CAPI. In order to provide a handy user-facing API, a couple of the C++ implementations are wrapped in pure Python to provide a nicer interface: The Evaluator constructor is wrapped in Python to also set up a logger and diagnostic handler that uses the logger. This allows the CAPI and C++ implementation to avoid dealing with Diagnostics. The CAPI simply returns null on failure, and the C++ implementation simply throws a Python error in this case. The pure Python diagnostic handler ensure the error and any notes are logged before the error is thrown. The Evaluator instantiate method is also wrapped in Python to handle a few things required to provide a handy, type-safe API. It takes care of accepting Python objects for actual parameters, and converting these to Attributes using an existing helper function. It does a similar conversion back to Python objects for primitive fields. It also handles some minor metaprogramming to inspect an incoming dataclass to determine its fields, extract them from the instantiated Object, and return an instance of the dataclass. Note that this initial implementation only supports Attributes in Object fields in the pure Python wrapper around instantiation. Support for Objects in Object fields will be added shortly in a subsequent patch.
* Fix stdout stream in comment * Import annotations from the future for type["Foo"] * Fix WARNING diagnostic severity * Fix varargs API and update test accordingly * Comments about Attribute <> Python
dc4f177
to
847f509
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
# Get the actual parameter Attributes from the supplied variadic | ||
# arguments. This relies on the circt.support helpers to convert from | ||
# Python objects to Attributes. | ||
actual_params = var_to_attribute(list(args)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this was what confused me initially, args
is actually a tuple not a list, but this is what I wanted--pass it directly to var_to_attribute
.
This adds the usual Python API structure and dialect registration boilerplate, as well as Python APIs around the Evaluator library.
The C++ implementations with pybind are intended to be as minimal and straightforward as possible, simply wrapping the CAPI.
In order to provide a handy user-facing API, a couple of the C++ implementations are wrapped in pure Python to provide a nicer interface:
The Evaluator constructor is wrapped in Python to also set up a logger and diagnostic handler that uses the logger. This allows the CAPI and C++ implementation to avoid dealing with Diagnostics. The CAPI simply returns null on failure, and the C++ implementation simply throws a Python error in this case. The pure Python diagnostic handler ensure the error and any notes are logged before the error is thrown.
The Evaluator instantiate method is also wrapped in Python to handle a few things required to provide a handy, type-safe API. It takes care of accepting Python objects for actual parameters, and converting these to Attributes using an existing helper function. It does a similar conversion back to Python objects for primitive fields. It also handles some minor metaprogramming to inspect an incoming dataclass to determine its fields, extract them from the instantiated Object, and return an instance of the dataclass.
Note that this initial implementation only supports Attributes in Object fields in the pure Python wrapper around instantiation. Support for Objects in Object fields will be added shortly in a subsequent patch.