GH-37537: [Integration][C++] Add C Data Interface integration testing#37769
GH-37537: [Integration][C++] Add C Data Interface integration testing#37769pitrou merged 4 commits intoapache:mainfrom
Conversation
pitrou
force-pushed
the
gh37537-c-data-integration
branch
from
ce9186f to
92cb90e
Compare
|
@wjones127 @tustvold, FYI, following this PR, you'll need to add the |
|
Thank you for the heads up - created a tracking issue so this doesn't get missed - apache/arrow-rs#4828 |
pitrou
requested review from
bkietz and
wjones127
and removed request for
assignUser and
raulcd
| raise NotImplementedError | ||
|
|
||
| def compare_allocation_state(self, recorded: object, | ||
| gc_until: typing.Callable[[_Predicate], bool] |
There was a problem hiding this comment.
I find this parameter's type quite confusing. IIUC it's a cancel token to cut off long-running gc? I'm not sure why this control is given to an exporter or importer. Instead, I would think it would make more sense for the runner check_memory_released to construct this token. It can be Callable[[], bool] and returns true if GC is taking too long and the runner is aborting the current case
There was a problem hiding this comment.
No, really, it's as the doc states. Some runtimes may need several GC calls to properly release memory, hence the name.
There was a problem hiding this comment.
I'm probably missing something, but it seems like this could be done more simply with something like:
@contextmanager
def check_memory_released(exporter: CDataExporter, importer: CDataImporter,
gc_timeout: Callable[[], bool] = _default_timeout):
do_check = (exporter.supports_releasing_memory and
importer.supports_releasing_memory)
if not do_check:
yield; return
before = exporter.record_allocation_state()
yield
while exporter.record_allocation_state() != before:
if gc_timeout():
raise ...
importer.gc_once()There was a problem hiding this comment.
It probably could, though I'm not sure it's simpler :-). Both solutions (yours and mine) are IMHO not very elegant, and we may have to revisit if making two GCs coexist ends up more complicated...
There was a problem hiding this comment.
I prefer mine for less inversion of control since the exporter doesn't need to manage the importer's garbage collection.
There was a problem hiding this comment.
I'll go with the current version. This can evolve quite easily as this is internal tooling, so no API guarantees.
github-actions
bot
added
awaiting changes
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
Overall this looks good. Have a few minor comments.
github-actions
bot
added
awaiting merge
Co-authored-by: Will Jones <[email protected]>
Co-authored-by: Will Jones <[email protected]>
github-actions
bot
added
awaiting changes
|
I'll merge this PR now, thanks for the reviews! |
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 3b646ad. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them. |
…esting (apache#37769) ### Rationale for this change Currently there are no systematic integration tests between implementations of the C Data Interface, only a couple ad-hoc tests. ### What changes are included in this PR? 1. Add Archery infrastructure for integration testing of the C Data Interface 2. Add implementation of this interface for Arrow C++ ### Are these changes tested? Yes, by construction. ### Are there any user-facing changes? No. * Closes: apache#37537 Lead-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Will Jones <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
…esting (apache#37769) ### Rationale for this change Currently there are no systematic integration tests between implementations of the C Data Interface, only a couple ad-hoc tests. ### What changes are included in this PR? 1. Add Archery infrastructure for integration testing of the C Data Interface 2. Add implementation of this interface for Arrow C++ ### Are these changes tested? Yes, by construction. ### Are there any user-facing changes? No. * Closes: apache#37537 Lead-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Will Jones <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
4 participants
Rationale for this change
Currently there are no systematic integration tests between implementations of the C Data Interface, only a couple ad-hoc tests.
What changes are included in this PR?
Are these changes tested?
Yes, by construction.
Are there any user-facing changes?
No.