-
Notifications
You must be signed in to change notification settings - Fork 392
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
Alternative design for node, argument handling in burn import #1840
Comments
Related: #1812 |
So I realized something about Node argument access that should have been obvious in hindsight. The node output(and graph_output) can be handled separately from graph input.
|
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The current design for handling of graph inputs and outputs leaves a bit to be desired in terms of complexity. I think it's important to consider the constraints. if there are constraints I haven't listed or that need to be changed, please let me know and I'll update them.
Implementation requirements
Required by spec or compiler
cast_update_outputs
,transpose_linear_weights
,check_constants
)Required for code generation
while these won't lead to a bug, they will lead to arguably incorrect code generation:
Desired for performance
Vec<Rc<RefCell<Node>>>
Desired for the API
My approach in #1795 is to completely separate nodes and arguments. Nodes store a key (which is the original, unchanged name), and updating an argument doesn't require any synchronization, but this leads to an undesirable api downstream
I had an idea for an alternative design where we could avoid separating nodes and arguments.
Alternative design
convert_node_proto
function, moving some of the stuff stored inOnnxGraphBuilder
into that struct.graph_io
is currently used, save muAdvantages
Problems
input mutation
This pattern relied on mutability being required only for graph outputs, an input can be added, swapped or dropped, but not mutated. Turns out there are 3 functions (
cast_update_outputs
,transpose_linear_weights
,check_constants
) that mutate their inputs. check_constant isn't a big deal because the node it's referencing will be deleted.for the other two, looking through the code on the main branch, I'm not seeing where the the changes to those mutated inputs were synced to the graph_io, so I'm a bit fuzzy as to whether those mutations actually happened to the original arguments, and if not how that mapped to a valid operation (the output of the previous node is correct as the input for the node in question)
arguably incorrect names
The second problem is some nodes will be remapped, and thus the names of the outputs will be invalid. less concerning is some constants will be removed, which will change the count. More concerning is you'll have some nodes (and arguments) named after the original type unless you either move anything that might remap to the conversion step (which sort of defeats the purpose), or find a way to split the logic for node transformations into parts for detection and transform.
Moving data from the functions/build steps that change the node to the proto_conversion while avoiding multiple mutable references to the same parent struct either involves passing extra arguments or returning some stuff and making node gen function slightly more complicated
We could drop this constraint, generating uuids for variable names, and this would make most of the steps independent. But that would almost certainly make debugging harder, and would make the generated code unreadable.
The text was updated successfully, but these errors were encountered: