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(pandas): support memtable in pandas backend #5927

Merged
merged 3 commits into from
Apr 6, 2023

Conversation

anjakefala
Copy link
Contributor

Closes #5467

(Relatively new contributor, and welcome feedback on anything nuanced that I am missing here!)

@anjakefala
Copy link
Contributor Author

anjakefala commented Apr 5, 2023

I see that I have some tests to remove notimpl from; I had only checked in test_generic.py.

It seems I should also remove them for the dask backend.

@anjakefala anjakefala marked this pull request as draft April 6, 2023 00:02
@cpcloud
Copy link
Member

cpcloud commented Apr 6, 2023

Thanks for the PR!

Except for the XPASSing tests, which are hopefully pretty easy to fix (just remove the corresponding backend from the list!) this looks good to me!

@cpcloud cpcloud added this to the 5.1 milestone Apr 6, 2023
@cpcloud cpcloud added feature Features or general enhancements pandas The pandas backend dask The Dask backend labels Apr 6, 2023
@anjakefala
Copy link
Contributor Author

Locally, I still have this failure: FAILED ibis/backends/tests/test_client.py::test_interactive_repr_max_columns[pandas-False] - AssertionError: assert ' c_19 ' not in '┏━━━━━━━┳━━...─────┴───┘\n'

I want to see if it also fails in the CI, and I will take a look!

@anjakefala anjakefala marked this pull request as ready for review April 6, 2023 16:47
@cpcloud
Copy link
Member

cpcloud commented Apr 6, 2023

I tried out

pytest 'ibis/backends/tests/test_client.py::test_interactive_repr_max_columns[pandas-False]'

and that passes for me.

@cpcloud
Copy link
Member

cpcloud commented Apr 6, 2023

commitlint is failing because

tests(pandas): memtable for pandas backend is now supported

should be

test(pandas): ...

Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Couple of micronits, not blocking.

Merging on green!

@anjakefala
Copy link
Contributor Author

Also, feel free to squash this PR!

@cpcloud
Copy link
Member

cpcloud commented Apr 6, 2023

I reported the pytest failure upstream: pytest-dev/pytest#10874.

@cpcloud
Copy link
Member

cpcloud commented Apr 6, 2023

No need to squash!

@anjakefala
Copy link
Contributor Author

anjakefala commented Apr 6, 2023

As a general FYI, I was also running into a DeprecationWarning with one of our dependencies and pytest locally, that I worked around with pytest -sv -m pandas ibis/backends/tests/ -W ignore::DeprecationWarning

Example:

ibis/backends/tests/test_temporal.py:34: in <module>
    from google.api_core.exceptions import BadRequest as GoogleBadRequest
../../miniconda3/envs/ibis-dev/lib/python3.10/site-packages/google/api_core/exceptions.py:29: in <module>
    from google.rpc import error_details_pb2
../../miniconda3/envs/ibis-dev/lib/python3.10/site-packages/google/rpc/__init__.py:18: in <module>
    import pkg_resources
../../miniconda3/envs/ibis-dev/lib/python3.10/site-packages/pkg_resources/__init__.py:3324: in <module>
    def _initialize_master_working_set():
../../miniconda3/envs/ibis-dev/lib/python3.10/site-packages/pkg_resources/__init__.py:3298: in _call_aside
    f(*args, **kwargs)
../../miniconda3/envs/ibis-dev/lib/python3.10/site-packages/pkg_resources/__init__.py:3349: in _initialize_master_working_set
    tuple(dist.activate(replace=False) for dist in working_set)
../../miniconda3/envs/ibis-dev/lib/python3.10/site-packages/pkg_resources/__init__.py:3349: in <genexpr>
    tuple(dist.activate(replace=False) for dist in working_set)
../../miniconda3/envs/ibis-dev/lib/python3.10/site-packages/pkg_resources/__init__.py:2870: in activate
    declare_namespace(pkg)
../../miniconda3/envs/ibis-dev/lib/python3.10/site-packages/pkg_resources/__init__.py:2338: in declare_namespace
    warnings.warn(msg, DeprecationWarning, stacklevel=2)
E   DeprecationWarning: Deprecated call to `pkg_resources.declare_namespace('sphinxcontrib')`

@cpcloud cpcloud enabled auto-merge (rebase) April 6, 2023 18:32
@cpcloud cpcloud disabled auto-merge April 6, 2023 18:32
@cpcloud
Copy link
Member

cpcloud commented Apr 6, 2023

That's something we can address in a separate PR, we usually do that by including a specific pattern after the "ignore" string in pyproject.toml so that we aren't propagating a bunch of useless warning spam to users from other libraries.

@cpcloud
Copy link
Member

cpcloud commented Apr 6, 2023

It doesn't seem to be triggering the failure in CI or locally for me, so it could be an environment issue.

@cpcloud
Copy link
Member

cpcloud commented Apr 6, 2023

🚢-ing it!

@cpcloud cpcloud merged commit ed08e44 into ibis-project:master Apr 6, 2023
@anjakefala anjakefala deleted the kef/5467 branch April 6, 2023 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dask The Dask backend feature Features or general enhancements pandas The pandas backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(pandas): support memtable in pandas backend
2 participants