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

fix(python): Replacing nest-asyncio with greenlet in database dependencies #17811

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

phi-friday
Copy link
Contributor

The nest-asyncio has been deprecated.
I replaced it with using threads instead, and in the case of sqlalchemy, using greenlet.

I specified greenlet as an optional dependency,
but it is also an implicit dependency of sqlalchemy when using asynchronous sessions.

@github-actions github-actions bot added fix Bug fix python Related to Python Polars labels Jul 23, 2024
@alexander-beedie
Copy link
Collaborator

Looks like very nice work! I was not looking forward to updating this 😄

Should close #17334? Can you confirm that it plays nice with Notebooks, and inside uvloop? (I'll start checking the PR in more detail later today or tomorrow, but if you've already taken a look at these cases -or could try them out?- that would be great).

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 82.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 80.37%. Comparing base (ca6d46c) to head (35113ae).
Report is 1 commits behind head on main.

Files Patch % Lines
py-polars/polars/io/database/_utils.py 81.57% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #17811      +/-   ##
==========================================
- Coverage   80.39%   80.37%   -0.02%     
==========================================
  Files        1496     1496              
  Lines      197480   197507      +27     
  Branches     2820     2822       +2     
==========================================
- Hits       158764   158750      -14     
- Misses      38194    38234      +40     
- Partials      522      523       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phi-friday
Copy link
Contributor Author

Looks like very nice work! I was not looking forward to updating this 😄

Should close #17334? Can you confirm that it plays nice with Notebooks, and inside uvloop? (I'll start checking the PR in more detail later today or tomorrow, but if you've already taken a look at these cases -or could try them out?- that would be great).

it works in local with uvloop

from __future__ import annotations

import sqlite3
import tempfile
from datetime import date
from pathlib import Path
from typing import Any

import uvloop
from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine

import polars as pl
from polars.testing import assert_frame_equal


def tmp_sqlite_db(test_db: Path) -> Path:
    def convert_date(val: bytes) -> date:
        return date.fromisoformat(val.decode())

    sqlite3.register_converter("date", convert_date)
    conn = sqlite3.connect(test_db)

    conn.executescript(
        """
        CREATE TABLE IF NOT EXISTS test_data (
            id    INTEGER PRIMARY KEY,
            name  TEXT NOT NULL,
            value FLOAT,
            date  DATE
        );
        REPLACE INTO test_data(name,value,date)
          VALUES ('misc',100.0,'2020-01-01'),
                 ('other',-99.5,'2021-12-31');
        """
    )
    conn.close()
    return test_db


def test_read_async(tmp_sqlite_db: Path) -> None:
    async_engine = create_async_engine(f"sqlite+aiosqlite:///{tmp_sqlite_db}")
    async_connection = async_engine.connect()
    async_session = async_sessionmaker(async_engine)
    async_session_inst = async_session()

    expected_frame = pl.DataFrame(
        {"id": [2, 1], "name": ["other", "misc"], "value": [-99.5, 100.0]}
    )
    async_conn: Any
    for async_conn in (
        async_engine,
        async_connection,
        async_session,
        async_session_inst,
    ):
        if async_conn in (async_session, async_session_inst):
            constraint, execute_opts = "", {}
        else:
            constraint = "WHERE value > :n"
            execute_opts = {"parameters": {"n": -1000}}

        df = pl.read_database(
            query=f"""
                SELECT id, name, value
                FROM test_data {constraint}
                ORDER BY id DESC
            """,
            connection=async_conn,
            execute_options=execute_opts,
        )
        assert_frame_equal(expected_frame, df)


async def main() -> None:
    with tempfile.TemporaryDirectory() as temp_dir:
        sqlite_db = Path(temp_dir) / "test.db"
        tmp_sqlite_db(sqlite_db)
        test_read_async(sqlite_db)

    print("done")


if __name__ == "__main__":
    uvloop.run(main())

output:

❯ rye run python ./test_uvloop.py
done

@phi-friday
Copy link
Contributor Author

also, in notebook, it works.
스크린샷 2024-07-23 오후 11 13 43

@alexander-beedie
Copy link
Collaborator

Nice! I'll experiment using some other non-alchemy async drivers shortly (such as SurrealDB), but if it's working everywhere else than I'm optimistic ;)

@ritchie46
Copy link
Member

@alexander-beedie can you take a look at this one?

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Aug 2, 2024

@alexander-beedie can you take a look at this one?

Yup; have just been trying to create a stripped-down test case of an example I've found where the new code hangs indefinitely 🤔

@phi-friday phi-friday force-pushed the feat-greenlet-async branch from b772ce2 to 8cc1dbd Compare August 6, 2024 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants