-
Notifications
You must be signed in to change notification settings - Fork 35
Out-Of-Process Interpreter for CppInterOp #717
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
Out-Of-Process Interpreter for CppInterOp #717
Conversation
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 16. Check the log or trigger a new build to see more.
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.
clang-tidy made some suggestions
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.
clang-tidy made some suggestions
c56d565 to
7217df9
Compare
|
Before pushing commits to this PR please either ping me, or delete caches as they are made. This PR has caused a large number of llvm cached builds to be deleted on main, which in turn has required multiple PRs workflow runs to be stopped while the cache is rebuilt. |
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.
clang-tidy made some suggestions
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #717 +/- ##
==========================================
- Coverage 79.39% 79.04% -0.36%
==========================================
Files 9 9
Lines 3780 3879 +99
==========================================
+ Hits 3001 3066 +65
- Misses 779 813 +34
🚀 New features to boost your workflow:
|
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.
Please look at the comments.
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.
clang-tidy made some suggestions
The tests are not covering over 1/4 of this patch. @kr-2003 can you improve the coverage? |
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.
You have 65 commits in this one single PR. Out of which ~15 says DO NOT MERGE. Now I am not sure if you want to merge this?
I changed this to "Draft PR" because of the "DO NOT MERGE" in the commit messages.
Can the coverage of this PR be increased? Over a 1/4 of it is not covered by tests. |
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.
clang-tidy made some suggestions
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.
clang-tidy made some suggestions
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.
This looks good to me.
I am going ahead and merging this PR. If there are any issues that are found, we can fix it in subsequent PRs. If any refactors are required, we can open issues for them.
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.
thanks for the work here @kr-2003
@anutosh491 @Vipul-Cariappa I would have liked better coverage for the PR, but am fine with this being merged now, subject to into being squashed and merged, since it has 73 commits, 16 of which say do not merge in the commit message. Just wait for the ci to go green first (ignoring the coverage job which will fail due to lack of coverage) @kr-2003 thanks for all your work getting this PR ready. |
Dismissing my request for changes, subject to the squash and merge mentioned in this message #717 (comment)
|
@anutosh491 @Vipul-Cariappa before you merge this. Please ping me here or on Discord, since I will need to delete the llvm cache builds this PR makes, and delete now outdated caches on main just before it goes in. |
| CLANG_VERSION="${{ matrix.clang-runtime }}" | ||
| if [[ "$CLANG_VERSION" == "20" && "${{ matrix.cling }}" == "Off" ]]; then | ||
| SUPPRESSION_FILE="../etc/clang${CLANG_VERSION}-valgrind.supp" | ||
| valgrind --show-error-list=yes --track-origins=yes --error-exitcode=1 \ | ||
| --show-leak-kinds=definite,possible \ | ||
| --suppressions="${SUPPRESSION_FILE}" \ | ||
| unittests/CppInterOp/CppInterOpTests/unittests/bin/${{ env.BUILD_TYPE }}/CppInterOpTests | ||
| else | ||
| valgrind --show-error-list=yes --track-origins=yes --error-exitcode=1 \ | ||
| --show-leak-kinds=definite,possible \ | ||
| unittests/CppInterOp/CppInterOpTests/unittests/bin/${{ env.BUILD_TYPE }}/CppInterOpTests | ||
| fi |
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.
@kr-2003 I believe this Valgrind command change will need to be done in the cppyy repos ci to stop them failing once this is in. Can you open up a PR in the CPyCppyy, cppyy-backend and cppyy repos with this once this PR is in?
This PR enables Out-Of-Process Interpreter for CppInterOp.
Under the hood, it uses LLVM's
llvm-jitlink-executorandlib ORC RTto delegate the JIT execution to different process. The user just needs to pass--use-oop-jitflag as ClangArgs while constructing the interpreter to use this feature.