Add Async Support #49

biniona · 2024-09-28T21:49:29Z

Fixes #9.

Adds Async Support to Minject with the following API:

@async_context
@inject.bind(text = "hi")
class MyClass:
    def __init__(self, text : str):
        self.text = text
        self.in_context = False

    async def __aenter__(self : Self) -> Self:
        self.in_context = True
        return self 
    
    async def __aexit__(self : Self, exc_type : Type[BaseException], exc_value : BaseException, traceback : TracebackType) -> None:
        self.in_context = False

async def main():
    my_class : MyClass
    async with Registry() as r:
        my_class = await r.aget(MyClass)
        assert my_class.text == "hi"
        assert my_class.in_context == True
    
    assert my_class.in_context == False

This PR only adds implementation + tests. Once we agree on implementation I can add docs.

Verification Steps

Doc Strings
Created Issue to Document feature

biniona · 2024-09-29T21:53:13Z

Tests passing as of here, but there are still bugs. There is some weirdness in navigating how top-level instantiated classes don't ever call the aresolve method.

An issue that I think is still in the code is that the following code would enter the context of MyClass twice.

registry.aget(MyClass)
registry.aget(MyClass)

biniona · 2024-09-30T22:22:08Z

Starting with a really rough implementation. It's pretty much working but has a lot of problems.

I think a lot of the issues are being caused by the distinction between a binding/reference and a top level class. It's creating a lot of duplication of code.

biniona · 2024-09-30T22:26:51Z

Passing checks as of 173156c. Still a lot of clean up though.

I think to do this correctly would require a fair amount of refactoring, and may involve wrapping the initial object to retrieve from the registry in an inject.reference(MyClass) call.

biniona · 2024-10-01T20:51:36Z

All checks passing here ^

biniona · 2024-10-01T21:01:13Z

minject/registry.py

@@ -180,6 +219,32 @@ def _register_by_metadata(self, meta: RegistryMetadata[T]) -> RegistryWrapper[T]

        return wrapper

+    async def _aregister_by_metadata(self, meta: RegistryMetadata[T]) -> RegistryWrapper[T]:


There may be a way to merge _aregister_by_metadata and _register_by_metadata using a context manager/generator, but this seemed like overkill to me.

Let me know if you disagree though.

biniona · 2024-10-01T21:03:49Z

minject/registry.py

+        by_meta = await self._aget_by_metadata(meta)
+        return _unwrap(by_meta)
+
+    def _get_class_if_registered(


NOTE: THIS METHOD IS THE ONE I AM MOST NERVOUS ABOUT IN THE PR

I added _get_class_if_registered to try and remove duplicated code in get(), _aget(), and aget() around checking if a class was already created in the registry.

This is the trickiest change to an existing implementation in this PR, and should probably be given significant attention around correctness in a review.

biniona · 2024-10-07T19:18:48Z

tests/test_async.py

+
+def test_get_sync_class_async_dependency_throws(registry: Registry) -> None:
+    with pytest.raises(AssertionError):
+        _ = registry.get(MySyncClassWithAsyncDependency, AUTO_OR_NONE)


The API for registry.get is a bit odd, you need to specify AUTO_OR_NONE to get the Registry to actually to create your class instance (see here). I think it makes sense to remove get from the public API for registry, and only push people towards using Registry.__getitem__.

Any objection to this @mmchenry-duolingo?

yes, get is probably unnecessary

xiyan128

Nice work! It looks great to me as long as we have follow-up PRs for documentation and @bcmills & @mmchenry-duolingo approve.

xiyan128 · 2024-10-07T19:33:14Z

minject/asyncio_extensions.py

+    # we must ignore it here.
+    from asyncio import to_thread  # type: ignore[attr-defined]
+# This is copy pasted from here: https://github.com/python/cpython/blob/03775472cc69e150ced22dc30334a7a202fc0380/Lib/asyncio/threads.py#L1-L25
+except ImportError:


(optional): not sure if it is a common practice to use try-except for feature detection. Shall we do something like below?

if hasattr(asyncio, 'to_thread'): from asyncio import to_thread

maybe this will solve the attr-defined type error?

Cool suggestion! I gave it a shot but it looks like it still raises the issue. I've seen this try/except before in base library code but I'm honestly not sure how widely used it is.

I'm alright keeping this as is for now, and if we find a better was to do it we can circle back to it.

mmchenry-duolingo

This looks like a reasonable beta approach to async to me, it fits pretty well with the existing patterns.

bcmills · 2024-10-08T19:02:06Z

minject/metadata.py

@@ -159,6 +174,16 @@ def _init_object(self, obj: T_co, registry_impl: "Registry") -> None:  # type: i

        self._cls.__init__(obj, **init_kwargs)

+    async def _ainit_object(self, obj: T_co, registry_impl: "Registry") -> None:  # type: ignore[misc]
+        """
+        asynchronous version of _init_object. Calls _aresolve instead


I agree that in general we can defer fixes for future maintainability, but In this case I'm requesting the doc comment in order to understand the code as a reviewer right now.

I'm having a hard time following the invariants around when the async objects are in what state, and having a more complete picture of what it means for an object to be “initialized” would help me better understand the rest of the code without having to spend as much time reverse-engineering it.

After spending some time digging into it, it appears that _ainit_object doesn't enter the context, as _push_async_context is called explicitly afterward.

bcmills · 2024-10-08T19:25:49Z

minject/registry.py

+            raise AssertionError("cannot use aget outside of async context")
+
+        meta = _get_meta_from_key(key)
+        maybe_class = self._get_if_already_in_registry(key, meta)


(nit) The name maybe_class suggests a class object, but from the return type of aget I expect this to be an instance of the class. I had to spend a while reading through the code to figure out which was the case — a clearer name would be helpful here.

Good catch.

Changed maybe_class to maybe_initialized_obj. Changed by_meta below to initialized_obj to highlight that this method is trying to give you usable objects from keys.

bcmills · 2024-10-08T19:41:18Z

minject/registry.py

@@ -177,6 +205,38 @@ def _register_by_metadata(self, meta: RegistryMetadata[T]) -> RegistryWrapper[T]

        return wrapper

+    async def _aregister_by_metadata(self, meta: RegistryMetadata[T]) -> RegistryWrapper[T]:
+        """
+        async version of _register_by_metadata. Calls _aiint_object instead of _init_object


(typo) s/_aiint/_ainit/

bcmills · 2024-10-08T19:50:02Z

minject/registry.py

@@ -86,9 +102,35 @@ def config(self) -> RegistryConfigWrapper:
    def resolve(self, key: "RegistryKey[T]") -> T:
        return self[key]

+    async def _aresolve(self, key: "RegistryKey[T]") -> T:


I'd prefer to not clean this up as part of this PR however in order to keep the PR focused and to increase the probability of hitting deadlines dependent on this PR.

In this case the reason I am asking for a doc comment is so that I can understand the code well enough to review it: I can complete the review more quickly if I don't have to spend as much time figuring out what everything does.

Here's what I think I've got so far:

_aresolve calls aget

aget calls _aget_by_metadata

_aget_by_metadata calls _aregister_by_metadata

_aregister_by_metadata calls _ainit_object

_ainit_object calls aresolve_value on the object's dependencies

aresolve_value calls aresolve

aresolve calls _aresolve (recursively).

bcmills · 2024-10-08T19:52:16Z

minject/registry.py

@@ -198,6 +258,17 @@ def _get_by_metadata(
        else:
            return None

+    async def _aget_by_metadata(self, meta: RegistryMetadata[T]) -> Optional[RegistryWrapper[T]]:
+        """
+        async version of _get_by_metadata. The default argument has been removed


I'm a little unclear on the relationship between _aget_by_metadata and _get_by_metadata. I see three calls to the latter (two in get and one in contains), but only one to the former (in aget).

I guess that's related in some way to the lack of a default argument?

I'm having a hard time seeing how __contains__ works for async objects — is _get_by_metadata also intended to be used for pre-registered async objects?

I completely wasn't thinking about __contains__. Great catch.

I believe __contains__ works as expected (added test to confirm this as I wasn't sure), but I agree this is hard to reason about.

I'll make a refactor issue to document this confusion. I think some of the trickiness here stems from the fact that _get_by_metadata is used for checking existence and creating things that don't yet exist. I think a possible refactor to split this up might be worthwhile. This would also make it clearer which parts of the code actually need to have sync/async alternates.

Issue is here: #56

bcmills · 2024-10-08T20:03:02Z

minject/registry.py

+        if not isinstance(key, type):
+            return None
+
+        # If the class has no metadata, but a parent has been registered in registry,


I'm having trouble understanding what's happening here. It seems like we're explicitly not checking the parents (include_bases=False)?

But then we are checking _by_iface, which will give us some type that implements (i.e. is a “child” of) T. So I think the code is correct but the comment is backwards..?

I believe that _by_interfaces is populated by meta.interfaces which is itself calculated as [cls for cls in inspect.getmro(cls) if cls is not object]. Based on this, I believe _by_interfaces contains the interfaces key implements (I.E. similar to parent classes), rather than objects that implement/extend key.

So I think "check parents" is close but not totally right. I've updated the comment to be more explicit about "an object exists in the registry that implements an interface the class implements...".

Agree this is all very tricky. Trying to think of a refactor idea to spin off this conversation. Unsure of one but if I can think of one I'll add an issue.

I chatted about this with @bcmills, and my understanding of the _by_iface look-up is most likely flawed. I was assuming that the _by_iface lookup looks for objects THAT implement the interfaces key implements, however, we should be looking for concrete subtypes of key. This makes more sense, as a key subtype could be swapped for the key type.

I'll update the comment to fix this after I've reread the relevant code (that I previously misread).

bcmills · 2024-10-08T20:06:42Z

minject/registry.py

+            )
+        self._async_entered = False
+        # close all objects in the registry
+        await self._async_context_stack.aclose()


What happens if any of the __aexit__ methods on the stack ends up raising an exception? (Does the call to self.close need to go in a finally: block?)

Great catch, agree this should be in a finally block. I'll add a test for this now to ensure that close methods are called if error is thrown during closing async context.

biniona added 6 commits September 28, 2024 17:32

add test for api - test not passing

a3c36ed

add more tests to pass

2e52bc9

wip - broken

97eab3c

passing all tests. Very messy but technical works as of this commit.

772dc1f

more tests

08b3348

pre-commit formatting

50ac88c

biniona added 6 commits September 30, 2024 18:00

add failing test - too many parent instantiations

16336bc

only instantiate class once

6108222

proofread

2cf7794

typing errors

4ce0c74

fix python3.7 error

41c9a4e

add hack type checking

838d369

fix type error

173156c

biniona added 7 commits September 30, 2024 23:49

simplify aget

a47911c

docs

08400a9

tests + docs + refactor

6f8bd63

add test + async plugin

2e24f28

docs

0d2245c

refactor interface check

82a1332

typing error

d2d655a

biniona commented Oct 1, 2024

View reviewed changes

biniona added 3 commits October 1, 2024 18:22

refactor

b53d559

proofread

22c73fb

changelog + version + refactor

f00b433

biniona assigned bcmills Oct 1, 2024

biniona added 4 commits October 7, 2024 14:54

ignore python 3.7 error + docs

cc4c5e9

ignore mypy errors - redefining signature

ee30e3c

proofread comment

8e7d796

add sync -> async failure tests

f18dd14

biniona commented Oct 7, 2024

View reviewed changes

biniona marked this pull request as ready for review October 7, 2024 19:19

biniona assigned bcmills, mmchenry-duolingo and xiyan128 and unassigned bcmills, xiyan128 and mmchenry-duolingo Oct 7, 2024

xiyan128 approved these changes Oct 7, 2024

View reviewed changes

mmchenry-duolingo approved these changes Oct 8, 2024

View reviewed changes

bcmills reviewed Oct 8, 2024

View reviewed changes

bcm - call close on error

cd13e39

biniona mentioned this pull request Oct 9, 2024

Get/Set Metadata is Hard to Reason About #56

Open

bcm - __contains__ test

9e64c57

biniona mentioned this pull request Oct 9, 2024

Simplify Mutual Recursion in Resolving Bindings to Objects #57

Open

biniona added 2 commits October 9, 2024 14:40

bcm - typo + var name

dabb7da

clarify comment

ae6c0d3

biniona assigned bcmills and unassigned bcmills Oct 9, 2024

bcmills approved these changes Oct 9, 2024

View reviewed changes

bcm - fix comment

537b1a2

biniona merged commit a5b216a into master Oct 9, 2024
4 checks passed

biniona mentioned this pull request Oct 9, 2024

Document Async API #59

Open

bcmills unassigned bcmills, mmchenry-duolingo and xiyan128 Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Async Support #49

Add Async Support #49

biniona commented Sep 28, 2024 •

edited

Loading

biniona commented Sep 29, 2024

biniona commented Sep 30, 2024

biniona commented Sep 30, 2024

biniona commented Oct 1, 2024

biniona Oct 1, 2024

biniona Oct 1, 2024

biniona Oct 7, 2024 •

edited

Loading

mmchenry-duolingo Oct 8, 2024

xiyan128 left a comment

xiyan128 Oct 7, 2024 •

edited

Loading

xiyan128 Oct 7, 2024

biniona Oct 9, 2024 •

edited

Loading

mmchenry-duolingo left a comment

bcmills Oct 8, 2024

bcmills Oct 8, 2024

biniona Oct 9, 2024

bcmills Oct 8, 2024

bcmills Oct 8, 2024

bcmills Oct 8, 2024

biniona Oct 9, 2024 •

edited

Loading

biniona Oct 9, 2024

bcmills Oct 8, 2024

biniona Oct 9, 2024 •

edited

Loading

biniona Oct 9, 2024 •

edited

Loading

bcmills Oct 8, 2024

biniona Oct 9, 2024 •

edited

Loading

		@@ -180,6 +219,32 @@ def _register_by_metadata(self, meta: RegistryMetadata[T]) -> RegistryWrapper[T]

		return wrapper

		async def _aregister_by_metadata(self, meta: RegistryMetadata[T]) -> RegistryWrapper[T]:

Add Async Support #49

Add Async Support #49

Conversation

biniona commented Sep 28, 2024 • edited Loading

Verification Steps

biniona commented Sep 29, 2024

biniona commented Sep 30, 2024

biniona commented Sep 30, 2024

biniona commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biniona Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiyan128 left a comment

Choose a reason for hiding this comment

xiyan128 Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biniona Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

mmchenry-duolingo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biniona Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biniona Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

biniona Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biniona Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

biniona commented Sep 28, 2024 •

edited

Loading

biniona Oct 7, 2024 •

edited

Loading

xiyan128 Oct 7, 2024 •

edited

Loading

biniona Oct 9, 2024 •

edited

Loading

biniona Oct 9, 2024 •

edited

Loading

biniona Oct 9, 2024 •

edited

Loading

biniona Oct 9, 2024 •

edited

Loading

biniona Oct 9, 2024 •

edited

Loading