-
Notifications
You must be signed in to change notification settings - Fork 26
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
[c++] Modify ManagedQuery
to perform async queries
#1953
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.
LGTM but I am not very deep into std::future<>
so merely commenting for now.
I am also a little fuzzy about when we actually launch multiple queries at once -- I had the (wrong, clearly) mental model that we launch one, and iterate if its status is incomplete.
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 needs to be branched off main
and then we can backport to release-1.6
At present it's branched off @mojaveazure 's feature-dev work which is unrelated to, and should not block, the 1.6.0 release we're trying to get out today
Good catch @johnkerl -- I only saw 'one commit' and nodded and was asleep as to the branch it came from. |
f869440
to
2443fc7
Compare
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1953 +/- ##
===========================================
- Coverage 62.81% 48.28% -14.54%
===========================================
Files 106 72 -34
Lines 10037 6464 -3573
===========================================
- Hits 6305 3121 -3184
+ Misses 3732 3343 -389
Flags with carried forward coverage won't be shown. Click here to find out 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.
LGTM! One minor suggestion.
bca4b2b
to
3ab343c
Compare
* Addition of async queries * Modify Python CI workflow * Check validity of `query_future_`
* Addition of async queries * Modify Python CI workflow * Check validity of `query_future_` Co-authored-by: nguyenv <[email protected]>
* [r] tiledbsoma-r 1.6.0 * Fix versions in news headers * try @nguyenv's #1953 * rerun CI * debug try 3 --------- Co-authored-by: Aaron Wolen <[email protected]>
Issue and/or context:
Across both APIs we have observed the
SOMAArray
intermittenly segfault when accessing the array. This occurs due to a race condition where we have multiple read queries and one query closes the array before the other queries have completed. When a query that has not completed tries to access a closed array, it segfaults. The solution is to modify the C++ManagedQuery
so that it performs async queries and waits for all query threads to complete before closing theSOMAArray
.Changes:
NDArray
read path #1817