-
Notifications
You must be signed in to change notification settings - Fork 84
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
ThunderFX: Interface UX #1529
Comments
As @mruberry has suggested, we can wrap it like this, WDYT @t-vi @IvanYashchuk
|
Note from triage today: we should follow-up with thinking about how we explain the I filed #1532 to track that issue, which we can separate from these proposed changes. |
This is a good idea. What about the following tweak?
That would let all arguments be specified like regular kwargs, and I think it would slightly cleanup the ThunderCompiler dunder init function, too. This approach might have some issues in the future because:
I think these are OK problems to have, and we can address them if they come up in the future. One refinement of this scheme would be to explicitly list known kwargs in the signature, so the signature could include kwargs for "fullgraph" and thunder options, for example. That would make the signature more readable if anyone looked at it. Then the ThunderCompiler dunder init could also accept some kwargs explicitly, which might be clearer. |
One thing to note is that So the UX should consider this object as well. Few options I can think of -
Irrespective of the approach, I think my main point is to make sure that this information is still easily accessible. lightning-thunder/thunder/dynamo/utils.py Lines 103 to 122 in 087637f
|
Yes, Also I noticed in some cases users use it like:
and it seems in this case it's easier to use |
Adding attributes to the returned callable, or creating new methods to access this information, both sound like great ideas that we can follow-up with in the future. It would also be nice to consider how those functions compare to the existing functions for accessing thunder.jit data, like: lightning-thunder/thunder/__init__.py Line 827 in 9de5434
But we can also work on aligning these options later, too |
It would be nice if instead of having to write
We instead allowed practitioners to write
Notebooks like this are a good opportunity to identify areas for improvement in our UX like this. We can create the thunderfx function so it accepts
**kwargs
to pass to the torch.compile call, too.It'd be great to see a PR changing this so we can update this doc.
Reply via ReviewNB
Originally posted by @mruberry in #1524 (comment)
cc @Borda
The text was updated successfully, but these errors were encountered: