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

feat(polars): allow user to specify "engine" kwarg #10151

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

deepyaman
Copy link
Contributor

Description of changes

Modeled after support for "streaming" kwarg, but I have a couple questions:

  1. I don't see any tests for "streaming", so I didn't add any for "engine"; is this fine?
  2. I didn't see any attempt to support older Polars versions that didn't accept "streaming", so I didn't try to do that for "engine", either. However, Polars is technically stable now; should we not force people to get latest?

@deepyaman deepyaman marked this pull request as ready for review September 17, 2024 22:49
@cpcloud
Copy link
Member

cpcloud commented Sep 18, 2024

We don't have tests for streaming, but we have a blog post that uses it, so it has had coverage.

It would be good to verify this behavior does something in the test suite. It can be a smoke test.

@jcrist
Copy link
Member

jcrist commented Sep 18, 2024

Maybe to avoid requiring a GPU for support we could do a mocktest just to ensure we're plumbing the engine kwarg through properly? Then if things fail we at least know it's not on our end.

@cpcloud
Copy link
Member

cpcloud commented Sep 18, 2024

Yeah, using mocker.spy would be sufficient IMO.

@deepyaman
Copy link
Contributor Author

Yep, sounds good, I was planning to mock it (and maybe actually go ahead and do the same for making sure streaming gets passed, too).

@deepyaman deepyaman self-assigned this Sep 19, 2024
ibis/backends/polars/tests/test_client.py Outdated Show resolved Hide resolved
ibis/backends/polars/tests/test_client.py Outdated Show resolved Hide resolved
@cpcloud cpcloud added this to the 10.0 milestone Sep 23, 2024
@cpcloud cpcloud added feature Features or general enhancements polars The polars backend labels Sep 23, 2024
@cpcloud cpcloud enabled auto-merge (squash) September 23, 2024 09:37
@github-actions github-actions bot added the tests Issues or PRs related to tests label Sep 23, 2024
@cpcloud cpcloud merged commit 3877d6d into ibis-project:main Sep 23, 2024
80 checks passed
@deepyaman deepyaman deleted the feat/polars/support-engine-kwarg branch September 23, 2024 14:55
ncclementi pushed a commit to ncclementi/ibis that referenced this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Features or general enhancements polars The polars backend tests Issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants