- Notifications
You must be signed in to change notification settings - Fork6.2k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[core][cgraph] Fix illegal memory access of cgraph when used in PP#51734
Conversation
What's the behavior difference here? |
After this revert, the illegal memory access reported in#51596 is fixed. |
Yes, but why is the illegal memory access fixed? What is the meaningful behavior difference that was introduced in that PR? |
Discussed offline. The goal of this PR is revert and mitigation, I will follow up with further investigation and clean up the code. |
…ay-project#51734)This fixes the illegal memory access bug reported inray-project#51596The culprit PR isray-project#51305This PR technically just manually revertsray-project#51305 because it's hard tosimply `git revert` that commit, because we now enforce that there areno core->air dependencies in CI. Seeray-project#51699This PR simply reverts to the original logic beforeray-project#51305 , and makescode copy as needed. We wanted to first get the bug mitigated, and thenfollow up to simplify and tailor the logic of`utils.get_cuda_devices()`, as it can be tricky.## Related issue numberClosesray-project#51596---------Signed-off-by: Rui Qiao <ruisearch42@gmail.com>
Why are these changes needed?
This fixes the illegal memory access bug reported in#51596
The culprit PR is#51305
This PR technically just manually reverts#51305 because it's hard to simply
git revertthat commit, because we now enforce that there are no core->air dependencies in CI. See#51699This PR simply reverts to the original logic before#51305 , and makes code copy as needed. We wanted to first get the bug mitigated, and then follow up to simplify and tailor the logic of
utils.get_cuda_devices(), as it can be tricky.Related issue number
Closes#51596
Checks
Verified that itfixes#51596
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.