Skip to content

Allow threads - #15

Merged
roeap merged 3 commits into
roeap:mainfrom
adriangb:allow-threads
Aug 5, 2024
Merged

roeap merged 3 commits into
roeap:mainfrom
adriangb:allow-threads

Conversation

@adriangb

Copy link
Copy Markdown
Contributor

This should release the GIL and allow use in multiple threads.

I tested with this script:

from contextlib import contextmanager
import math
from time import time
from typing import Iterator

import anyio
import anyio.to_process
import anyio.to_thread
import object_store


@contextmanager
def timeit(name: str) -> Iterator[None]:
    start = time()
    yield
    print(f'{name} took {time() - start:.2f} seconds', flush=True)


def work() -> None:
    object_store.ObjectStore('gs://adriangb-public-bucket').get('yellow_tripdata_2024-01 (1).parquet')


async def awork(limiter: anyio.CapacityLimiter) -> None:
    await anyio.to_thread.run_sync(work, limiter=limiter)


async def main() -> None:
    limiter = anyio.CapacityLimiter(math.inf)

    with timeit('main'):
        async with anyio.create_task_group() as tg:
            for _ in range(32):
                tg.start_soon(awork, limiter)


if __name__ == '__main__':
    anyio.run(main)

Locally there isn't much difference, I'm IO bound. But on GCP compute that goes from ~15s to ~3s for me.

Comment on lines +511 to +517
fn put(&self, py: Python, location: PyPath, bytes: Vec<u8>) -> PyResult<()> {
py.allow_threads(|| {
self.rt
.block_on(self.inner.put(&location.into(), bytes.into()))
.map_err(ObjectStoreError::from)?;
Ok(())
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I always thought that if you didn't include the py: Python argument, that the GIL would be released automatically. Is this not true? To release the GIL you have to include the py argument and then also call allow_threads?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought the same thing but that's not what my test showed. If you look at the docs: https://pyo3.rs/v0.21.1/parallelism#parallelism

It goes from

fn search_sequential(contents: &str, needle: &str) -> usize {
    contents.lines().map(|line| count_line(line, needle)).sum()
}

To

fn search_sequential_allow_threads(py: Python<'_>, contents: &str, needle: &str) -> usize {
    py.allow_threads(|| search_sequential(contents, needle))
}

So I think we are wrong.

I can confirm with @davidhewitt tomorrow.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Indeed the GIL is not released automatically, as many arguments like Bound<'py, PyAny> can also implicitly carry the GIL even if py: Python<'py> is not included.

@adriangb

adriangb commented May 3, 2024

Copy link
Copy Markdown
Contributor Author

@roeap quick ping on this

@roeap

roeap commented Jun 9, 2024

Copy link
Copy Markdown
Owner

@adriangb - sorry for being MIA for so long. Do you mind rebasing, and I am happy to review / merge then.

@ion-elgreco

Copy link
Copy Markdown

@roeap will you push a 0.2.0 release after this one?

@roeap
roeap merged commit 1a2f6bc into roeap:main Aug 5, 2024
@adriangb
adriangb deleted the allow-threads branch August 5, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants