[ESI] Don't assume __dict__ attribute for all objects #8911
Conversation
... when checking if a given object is valid for `StructType` serialization. E.g., `bytes` objects do not have a `__dict__` attribute.
teqdruid
left a comment
teqdruid
left a comment
There was a problem hiding this comment.
Not strictly related to this PR: we've (I've) played a bit fast and loose with testing, especially the Python API. Now that we've got the ability to create Python types from Python (right?), we could create some pytests to test the type / value system without invoking PyCDE to create a system (manifest). This is even more important for Python APIs since they don't get compiled. I'm less worried about the C++ side as a result of this (and its functionality is significantly simpler) but we definitely need some mechanism to test that better also.
|
|
||
| def serialize(self, obj) -> bytearray: | ||
| ret = bytearray() | ||
| if not isinstance(obj, dict): |
There was a problem hiding this comment.
Is is_valid called before this? If not, it should be. Problem is that it should either be called at the top of the serialize call or be called here and not descend into the inner objects. Or is it better to require the user code to check and put it somewhere in the documentation?
There was a problem hiding this comment.
The main place where is_valid is called is when writing to a port. Since it (as you mention) is somewhat critical that a given type is_valid when trying to serialize and object, that is_valid call should probably be moved to the serialization function.
Completely agree. But, before that, we should probably have some dummy backend, such that we can test the ESI runtime proper. I.e., what it'd really like to be able to do is to just open the ESIRuntime subdirectory, and not care at all about building the rest of CIRCT to test it. |
We do have a dummy backend ( |
In my mind, a proper "dummy" backend also has APIs to dynamically construct the underlying accelerator (99% of the time, it's probably just loopback, but i can imagine an API of that dummy backend which takes an input type, output type, and a lambda, which defines the transformation in between). Then we take PyCDE/complexity out of the equation, and keep everything internral to the ESI runtime project. |
... when checking if a given object is valid for
StructTypeserialization. E.g.,bytesobjects do not have a__dict__attribute.Also fixes bug in struct serialization, where an object's
__dict__array wasn't being considered (even though it is foris_valid). That could e.g. make serializing adataclassto an ESI struct fail.