-
Notifications
You must be signed in to change notification settings - Fork 223
[Frontend] Refactor how frontend handles node datatypes #641
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
base: main
Are you sure you want to change the base?
Conversation
|
cc: @zhanghb97 @R-Tars |
|
Thanks for pointing this out — I agree it is an important issue. For now, I don’t think we should rush to unify In the longer term, I do think converging on |
|
I ran into a runtime issue when testing this PR locally with Torch 2.8. The build fails while generating the DeepSeek-R1 example, and the traceback points to In this case, |
|
Thanks for pointing that out, I have been struggling to validate whether the change will cause any breakages. I'll try to figure out something and maybe add frontend tests. |
|
Thanks for looking into this. Just to add some context: a recent commit has caused most models to fail at runtime, so the frontend is currently in a rather unstable state. We are currently trying to address this issue. As a result, even if this specific issue is fixed, models may still not run correctly at the moment. |
|
It seems that this PR has not been updated for some time. I pulled the latest changes today and tested them locally against the current main branch. At the moment, the DeepSeek-R1 example still cannot run successfully, so the runtime issue does not appear to be fully resolved yet. For this PR to be ready for merging, I think a minimum requirement is that the DeepSeek-R1 example can run correctly from start to finish. Currently, this is still not the case. If you encounter any difficulties fixing the issue, please feel free to let me know — I am happy to help with debugging. |
This PR implements a more direct reference system in
Graphto different node types (e.g.Input,Parametc.).Some notes about the current version of the draft.
GraphImporterstill also only receivesinputandparamsinTensorMeta(see ambiguity of this below). I've left it this way, because inGraphImporterwe do not expect to change this again, so there is no reason to introduce more complication, though the notation will be improved to reflect this.One point of friction here is that thorughout the frontend there seems to be two notions of
tensor_meta, one being the typeTensorMeta, the other being either adict[str, Any], the other beingdict[str, list[Any]]. For now I left some convenience script in for the sake of this draft, but let's discuss what the general direction with this should be.Closes #639