-
Notifications
You must be signed in to change notification settings - Fork 89
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
[Feature Request] Enable node attributes to be edited #198
Comments
Hmm... I'm not sure we want to scope creep into editing but let's see. @yijie-yang, any thoughts? |
Thank you for your suggestion, @madcampos! The idea of editing node attributes in Model Explorer is interesting. Could you share more about your workflow for testing and tweaking models? For instance, we typically make changes directly to the model and use Model Explorer to visualize and verify those changes. What specific benefits do you envision from editing within the visualization tool instead of directly in the model? Your insights would be valuable in understanding how we can enhance this feature. |
The idea is to provide a feedback loop to users, similar to a REPL. Where it shows on Model Explorer places where changes can be made, and this would provide a UI for changing those values. What can be changed is informed by the model, it would have metadata about what fiends can be changed and constraints to those fields like a list of candidate values or min and max values. When edits are made, then the user would send a command back to the server to regenerate the model with the applied changes. Adding @vprajapati-tt to the conversation as he can provide more insight and details. |
Thanks for your insights! I think the idea of allowing users to edit the model via the UI and then recompiling it based on those changes is great. To better understand your needs, could you clarify a couple of points?
Thanks! |
Thanks for looping me in Marco, let me try to take on these questions.
The proposed changes would ideally only implement the frontend changes and infrastructure in the adapter framework to allow for this functionality (should the extension developer desire). I think if model-explorer is left as a tool that is purely a "renderer", a graph-level manipulation can occur in the extension, and it is merely reflected in model-explorer as a new graph for the user to visualize the impact of their modification. The goal is not to create a pile of complex logic in model-explorer to handle this functionality, but rather form a contract between the adapter & model-explorer such that this functionality is accessible to developers. As an example, this contract could involve the addition of the following commands in an adapter that seeks editing functionality:
I would love to hear your thoughts and input on this, and I'm happy to hear you're considering it! |
Hi @vprajapati-tt , thanks for your detailed response. I'm working with @yijie-yang to explore model surgery opportunities in Model Explorer. Instead of recording user changes on Model Explorer UI and send it through a list of commands to update the MLIR in your adaptor, how about the idea of sending the whole updated Model Explorer graph along with the original model to adapter and let your adapter reconstruct a new MLIR module from it? |
Hey @chunnienc, the reason I proposed that method was in case there are adapter developers who require a more complex method of updating / changing the IR that they're working with. Reconstructing MLIR is a relatively trivial problem, but adding custom reconstruction logic for a more involved IR could be a higher barrier of entry into the tool compared to utilizing the same translation logic into a ModelExplorerGraph with an updated IR. Of course, the tool and it's intricacies are understood best by your team. I would leave all implementation details depending on the many stakeholders involved, these are just my thoughts on ease-of-use changes for adapter developers. |
As a way to test and tweak models, it would be interesting to add the ability to edit nodes and then send those changes back to the server for recompilation.
The text was updated successfully, but these errors were encountered: