Circumvent deep copy of context in PredictModelPlugin
#334
Conversation
Codecov Report
@@ Coverage Diff @@
## main #334 +/- ##
==========================================
- Coverage 95.74% 95.73% -0.01%
==========================================
Files 65 65
Lines 2841 2840 -1
Branches 429 429
==========================================
- Hits 2720 2719 -1
Misses 75 75
Partials 46 46
Continue to review full report at Codecov.
|
VibhuJawa
left a comment
VibhuJawa
left a comment
There was a problem hiding this comment.
Thanks for the work on this @charlesbluca . Looks reasonable to me . Have requested some testing to ensure we dont miss anything.
| sql_outer_query = tmp_context._to_sql_string(outer_select) | ||
| df = tmp_context.sql(sql_outer_query) |
There was a problem hiding this comment.
Seems reasonable to me but can we should a test to ensure we have not broken anything. Like are we sure that sql_outer_query does not add anything .
Maybe we add a test to verify that the registered tables still line up and nothing temporary is left around.
There was a problem hiding this comment.
Good idea, I will add that in now
VibhuJawa
left a comment
VibhuJawa
left a comment
There was a problem hiding this comment.
Thanks for the quick work on this @charlesbluca . LGTM.
3 participants
Closes #332
It looks like the relatively expensive deep copy of the context done in
PredictModelPluginshould be circumvented by simply adding the temporary table we create to the original context and dropping it before returning; this also unblocks usage with multi-GPU cuML models.cc @VibhuJawa