-
Notifications
You must be signed in to change notification settings - Fork 34
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
[Frontend] Support static_argnums
#476
Conversation
Always recompiler for now.
Hi @tzunghanjuang, thank you for opening this PR :) Please let us know when you feel it is ready to review, you can also tag me as a reviewer at that point. |
Hi @dime10, it is ready for review but it seems that I do not have repo permission to add you as the reviewer. Thank you very much. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #476 +/- ##
=======================================
Coverage 99.56% 99.57%
=======================================
Files 43 43
Lines 7655 7722 +67
Branches 516 533 +17
=======================================
+ Hits 7622 7689 +67
Misses 17 17
Partials 16 16 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job getting this feature working 💯
I have a couple of questions that ask for clarification and a few suggestions to update the code, but overall this is looking great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this @tzunghanjuang 🥳
After all discussions are closed it should be good to merge :)
Oh one last thing, most PRs require a changelog entry describing at a high level what changed and why. This PR would be a "new feature", and those also require a code example in the changelog entry demonstrating the new feature. Have a look at the file here: https://github.com/PennyLaneAI/catalyst/blob/main/doc/changelog.md |
Sorry about the formatting failure, I think the |
Co-authored-by: David Ittah <dime10@users.noreply.github.com>
Also disble shared_object.close() if static arguments exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tzunghanjuang, I posted my answer to the last remaining issue: #476 (comment)
And make CompiledFunction store worksapce to avoid cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job fixing the library reopening 👍
And rename recompilation_needed into recompilation_happened
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, this one is good to go, great work 🎉 🎉
Context: Supporting
static_argnums
to identify which arguments are static. IDescription of the Change: Introduce the
static_argnums
parameter to@qjit
and add a mechanism to determine if the function needs to be recompiled when the static arguments are changed.Benefits: Users can pass custom objects to a function with
@qjit
. TheQJIT
object also stores all previously compiled functions for different arguments. If the arguments are seen before, there is no need for re-compilation.Possible Drawbacks: The
QJIT
object will store all the previously compiled functions and require more space.The mechanism for recompilation is based on checking hash values, so there might be a possibility of collision. It also creates a new
catalyst.utils.filesystem.WorkspaceManager
for every new function. If we do not create a newWorkspaceManager
, the new function will not be properly compiled. I am not sure if I should modify anything related to it.Related GitHub Issues: closes #461