Skip to content
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

test(ci): ensure that we do not skip all of test_client.py if polars is not installed #9617

Merged
merged 10 commits into from
Jul 18, 2024

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Jul 17, 2024

Avoid skipping all of test_client.py when polars is not installed.

@cpcloud cpcloud added this to the 9.2 milestone Jul 17, 2024
@cpcloud cpcloud added tests Issues or PRs related to tests ci Continuous Integration issues or PRs labels Jul 17, 2024
@cpcloud cpcloud changed the title test(ci): ensure that we do not skip all of test_client.py if polars is not installed test(ci): ensure that we do not skip all of test_client.py if polars is not installed Jul 17, 2024
@cpcloud
Copy link
Member Author

cpcloud commented Jul 17, 2024

Confirmed that test_client.py is now running for Exasol: https://github.com/ibis-project/ibis/actions/runs/9973508235/job/27558993516?pr=9617#step:16:76

@cpcloud cpcloud force-pushed the exasol-test-client-in-ci branch from 704c9e2 to 0bc2d69 Compare July 17, 2024 15:18
@cpcloud cpcloud added the ci-run-cloud Run BigQuery, Snowflake, Databricks, and Athena backend tests label Jul 17, 2024
@ibis-docs-bot ibis-docs-bot bot removed the ci-run-cloud Run BigQuery, Snowflake, Databricks, and Athena backend tests label Jul 17, 2024
@cpcloud cpcloud force-pushed the exasol-test-client-in-ci branch from 96b4a88 to ed36aec Compare July 17, 2024 15:52

cached_df = cached_table.to_pandas()
backend.assert_frame_equal(non_cached_table.to_pandas(), cached_df)
assert sys.getrefcount(op) == 2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @jcrist for all things reference count related 😬

This assertion passes, but if you apply this diff (inlining the cached_table.to_pandas() call, then refcount assertion fails. Here's the diff:

diff --git a/ibis/backends/tests/test_expr_caching.py b/ibis/backends/tests/test_expr_caching.py
index 9551f4133..f38df40c4 100644
--- a/ibis/backends/tests/test_expr_caching.py
+++ b/ibis/backends/tests/test_expr_caching.py
@@ -73,8 +73,7 @@ def test_persist_expression_multiple_refs(backend, con, alltypes):
     op = non_cached_table.op()
     cached_table = non_cached_table.cache()
 
-    cached_df = cached_table.to_pandas()
-    backend.assert_frame_equal(non_cached_table.to_pandas(), cached_df)
+    backend.assert_frame_equal(non_cached_table.to_pandas(), cached_table.to_pandas())
     assert sys.getrefcount(op) == 2
 
     name = cached_table.op().name

Digging around a bit with gc.get_referrers, it looks like a stack frame from inside pyspark's pandas/conversion.py is holding a reference to cached_table in the inlined case but not when the DataFrame is lifted out into a variable.

Would appreciate any thoughts you have here. I'll probably keep the original code and xfail pyspark for now if there's no obvious way forward here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pushing some other commits, and I'll avoid force pushing for now. You'll want to poke around at 6d0971b.

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@cpcloud cpcloud merged commit ea592d0 into ibis-project:main Jul 18, 2024
82 checks passed
@cpcloud cpcloud deleted the exasol-test-client-in-ci branch July 18, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration issues or PRs tests Issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants