Skip to content

tooling: Async/pathlib/mypy cleanups and utils#17505

Merged
htuch merged 2 commits intoenvoyproxy:mainfrom
phlax:tooling-async-prop
Aug 4, 2021
Merged

tooling: Async/pathlib/mypy cleanups and utils#17505
htuch merged 2 commits intoenvoyproxy:mainfrom
phlax:tooling-async-prop

Conversation

@phlax
Copy link
Member

@phlax phlax commented Jul 27, 2021

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message: tooling: Improve async runner/test handling
Additional Description:

This Pr does a few things:

  • adds an async_property decorator which is v handy for writing async classes
  • switches pytest to -Werror which is kinda necessary for catching asyncio unawaited errors.
  • adds a dep pin for setuptools to base/utils - some environments were still using older versions of setuptools which can cause issues with -Werror (as well as other incompatibilities)
  • makes runner.catches work async
  • improves the main plugin test to work async
  • adds some tests for pytest plugin fixtures
  • mypy fixes/cleanups
  • os -> pathlib cleanups (Fix Use python pathlib more and clean up some existing code with it #17487 )

re the setuptools dep - i tried removing from docs requirements - but it wouldnt let me - setuptools works in mysterious ways

The decorator can be used as so:

import asyncio

from envoy.tools.base.functional import async_property


Class MyClass(object):

    @async_property
    async def myprop(self):
        return await some_other_coroutine()

    async def run(self):
        print(await self.myprop)

asyncio.run(MyClass().run())

it can also cache the results of the async call like so:

Class MyClass(object):

    @async_property(cache=True)
    async def myprop(self):
        return await some_other_coroutine()

in this case repeated awaits of the property will return the cached results.

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax marked this pull request as draft July 27, 2021 14:30
@phlax phlax force-pushed the tooling-async-prop branch 3 times, most recently from 38243f9 to bd2bdf9 Compare July 27, 2021 15:59
@phlax
Copy link
Member Author

phlax commented Jul 27, 2021

@phlax phlax force-pushed the tooling-async-prop branch 4 times, most recently from def7f0e to e6b0e08 Compare July 27, 2021 19:36
@phlax phlax changed the title [WIP] tooling: Add async_property method decorator tooling: Add async_property method decorator Jul 27, 2021
@phlax phlax marked this pull request as ready for review July 27, 2021 19:42
@phlax phlax force-pushed the tooling-async-prop branch 3 times, most recently from 454e737 to a18e626 Compare July 28, 2021 10:19
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jul 28, 2021
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #17505 was synchronize by phlax.

see: more, trace.

@phlax phlax force-pushed the tooling-async-prop branch from a18e626 to 16d400d Compare July 28, 2021 10:25
@phlax phlax changed the title tooling: Add async_property method decorator tooling: Add async_property method decorator (+ more cleanups) Jul 28, 2021
@phlax phlax force-pushed the tooling-async-prop branch from 16d400d to 5f46745 Compare July 28, 2021 10:40
@phlax phlax force-pushed the tooling-async-prop branch 3 times, most recently from a8fe24e to 13e6bd5 Compare July 28, 2021 15:53
@phlax phlax changed the title tooling: Add async_property method decorator (+ more cleanups) [WIP] tooling: Add async_property method decorator (+ more cleanups) Jul 29, 2021
@phlax phlax marked this pull request as draft July 29, 2021 17:46
@phlax
Copy link
Member Author

phlax commented Jul 29, 2021

ive figured out a better implementation that doesnt depend on gc for closing coroutines

WIP while i reimplement...

@phlax phlax force-pushed the tooling-async-prop branch 3 times, most recently from 81b911b to 6c820c7 Compare July 30, 2021 09:15
@phlax phlax force-pushed the tooling-async-prop branch 2 times, most recently from 0abc53e to 3640423 Compare August 1, 2021 13:01
@phlax phlax changed the title [WIP] tooling: Improve async runner/test handling + mypy cleanups [WIP] tooling: Async/pathlib/mypy cleanups and utils Aug 1, 2021
@phlax phlax force-pushed the tooling-async-prop branch 8 times, most recently from 534cae3 to aa51d7e Compare August 1, 2021 16:18
- add `async_property` method decorator
- make `runner.catches` work async
- general py/test async improvements
- mypy/typing cleanups
- `os` -> `pathlib` cleanups

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the tooling-async-prop branch from aa51d7e to 13045a2 Compare August 1, 2021 16:38
@phlax phlax changed the title [WIP] tooling: Async/pathlib/mypy cleanups and utils tooling: Async/pathlib/mypy cleanups and utils Aug 1, 2021
@phlax phlax marked this pull request as ready for review August 1, 2021 16:48
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Looks good, generally prefer much smaller PRs but in this case it seems fine as a cleanup rollup.

@phlax
Copy link
Member Author

phlax commented Aug 2, 2021

generally prefer much smaller PRs but in this case it seems fine as a cleanup rollup.

yep, apologies, it kinda morphed

also, mypy can be a bit of rabbit hole of type safety, and changing only some of the paths to pathlib was making it unhappy (as do functions/methods that can return more than one type)

@phlax
Copy link
Member Author

phlax commented Aug 2, 2021

updated

as an aside @htuch writing/testing so many props is making reconsider not using attrs - that is what it kinda gives you free along with class/prop validation/mutators etc - and works out of the box with mypy

on my travels through the interwebs i read gvr saying somewhere that python was in the process of adopting some of the attrs features so i need to investigate this a bit further

@phlax phlax force-pushed the tooling-async-prop branch from edaec32 to f0a9b1e Compare August 2, 2021 08:50
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the tooling-async-prop branch from f0a9b1e to f2c11cf Compare August 2, 2021 17:08
@phlax phlax requested a review from htuch August 3, 2021 06:57
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Epic, thanks!

@htuch
Copy link
Member

htuch commented Aug 4, 2021

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Aug 4, 2021
@htuch htuch merged commit 1b9c688 into envoyproxy:main Aug 4, 2021
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.

Use python pathlib more and clean up some existing code with it

2 participants