aio-libs / aiobotocore

asyncio support for botocore library using aiohttp
https://aiobotocore.rtfd.io
Apache License 2.0
1.14k stars 179 forks source link

update venv scripts #1074

Open thehesiod opened 7 months ago

thehesiod commented 7 months ago

ensures compiled requirements file always match sources used to generate it

thehesiod commented 7 months ago

@jakob-keller thoughts? this uses another aio-libs module which seems to do what we want

thehesiod commented 7 months ago

for some reason I can't add you as a reviewer

jakob-keller commented 7 months ago

for some reason I can't add you as a reviewer

I guess that's because I'm not a collaborator.

jakob-keller commented 7 months ago

@jakob-keller thoughts? this uses another aio-libs module which seems to do what we want

Good idea. I did not know about that library.

Do we understand the performance implications? AFAIK, S3 Express is all about lower latencies. It would be a shame, if that cache added more overhead than necessary. A benchmark would be great.

As a user, I would be slightly concerned about adding another dependency: Fewer is better, IMO. In particular, if there is another fairly straightforward solution available as #1073.

jakob-keller commented 7 months ago

Do we understand the performance implications? AFAIK, S3 Express is all about lower latencies. It would be a shame, if that cache added more overhead than necessary. A benchmark would be great.

Here's a simple benchmark.py:

import asyncio
import functools

import async_lru

async def main(acache, n: int = 1000) -> None:
    """Async main coroutine."""
    await asyncio.gather(*(acache("test") for _ in range(n)))

async def noop(_arg: str) -> None:
    """Trivial noop cooroutine."""

@functools.lru_cache(maxsize=100)
def _cache_1073(_arg: str) -> asyncio.Task[None]:
    return asyncio.create_task(noop(_arg))

async def cache_1073(_arg: str) -> None:
    """functools.lru_cache wraps asyncio.Tasks - #1073."""
    return await _cache_1073(_arg)

@async_lru.alru_cache(maxsize=100)
async def cache_1074(_arg: str) -> None:
    """async-lru based - #1074."""
    return await noop(_arg)

Run with

python -m timeit -s "import asyncio; from benchmark import main, cache_1073" "asyncio.run(main(cache_1073))"

vs.

python -m timeit -s "import asyncio; from benchmark import main, cache_1074" "asyncio.run(main(cache_1074))"

Under CPython 3.11.7 on macOS, #1073 turns out to be slightly faster than #1074: ~10%. That is not significant IMO and should not determine the decision.

thehesiod commented 7 months ago

@jakob-keller I don't think #1073 is correct, it's avoiding a needed lock to ensure each lru entry is only awaited once

thehesiod commented 7 months ago

IMO any extra effort should be put into speeding up/correcting https://github.com/aio-libs/async-lru. Looking at it I think there are many potential options. external deps aren't great indeed, but at least it's from the same ecosystem (aiolibs)

thehesiod commented 7 months ago

If I get some time I'll look into that module, perhaps we can inline something simpler too

jakob-keller commented 7 months ago

@jakob-keller I don't think #1073 is correct, it's avoiding a needed lock to ensure each lru entry is only awaited once

I'm still convinced that both approaches are valid fixes for #1072: see https://github.com/aio-libs/aiobotocore/pull/1073#discussion_r1456336296

jakob-keller commented 7 months ago

for some reason I can't add you as a reviewer

I guess that's because I'm not a collaborator.

Now I am. Thank you! 🎉

thehesiod commented 7 months ago

thanks for all the investigative work and instructiveness!

thehesiod commented 7 months ago

@jakob-keller ok re-engineered based on discussions

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (110b0a2) 86.13% compared to head (5749e2c) 86.13%. Report is 1 commits behind head on master.

:exclamation: Current head 5749e2c differs from pull request most recent head e5f5608. Consider uploading reports for the commit e5f5608 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1074 +/- ## ======================================= Coverage 86.13% 86.13% ======================================= Files 60 60 Lines 5863 5863 ======================================= Hits 5050 5050 Misses 813 813 ``` | [Flag](https://app.codecov.io/gh/aio-libs/aiobotocore/pull/1074/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/aio-libs/aiobotocore/pull/1074/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `86.13% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.