inject.attr: Returned AbstractContextManager is already exited

[obs-gh-peterkolloch]

#96 added special handling for _GeneratorContextManagers to inject.attr.

I think the logic is faulty and/or is not very useful since it "exits" the context manager before even returning it.

The example that the author provided with version 5.3.0 outputs:

enter
exit
20

and thus demonstrates that the context manager returned by inject.attr is already exited before usage.

Looking at the implementation, this is not a surprise:

python-inject/src/inject/__init__.py

Lines 288 to 290 in 6ff646b

	elif isinstance(inst, contextlib._GeneratorContextManager):
	with contextlib.ExitStack() as sync_stack:
	inst = sync_stack.enter_context(inst)

A new ExitStack is created, used with with which implicitly class __enter__/__exit__ before returning the new instance. Potentially the author thought that this is about exiting the Python program because of the potentially misleading name?

Anyways, that would also not be a good option since that would hold onto the instance and prevent its garbage collection.

After a quick brainstorming, I could think of the following options:

1. 🥇 remove the special logic for AbstractContextManager -- the caller of inject.attr must call enter/exit the context appropriately.
2. Instead of contextlib.ExitStack() use inject.instance(ExitStack). That way one can manage a "global" ExitStack and exit at the appropriate time. I don't like that too much, because again, one would have to close and remove the ExitStack to get rid of the instance.

What do you think? In the meantime, I'll work around this in wrapping the context managers so that they are not recognized as context managers anymore.

[pyctrl]

Hi there. Let me join this attr (and also attr_dc) discussion.

I agree with suggestion No. 1 about removing context manager behavior from attr.

I know it works with autoparams — it's a decorator where you have wrapping context to manage "before original func call" and "after original func call" stages.
But this context is unavailable during attribute access or get_instance (and useless — there is no scope to wrap it with "before"/"after").

So I'm ➕ 1️⃣ for topic starter.

[pyctrl]

The second thing about attr and attr_dc — they are mostly confusing (at least for me). There is not much docs about it (I can help if you'd like).

My perspective around attr-features:

• they are Descriptors
• Descriptors are commonly used to define some model fields (pydantic, marshmallow, Django, SQLAlchemy, etc...)
• Descriptor instances become class attributes (accessing them as class attributes MyModelClass.descriptor_attribute returns instance of descriptor instead of any value for class instance field)
• this fields receive their real values during instance attribute access my_model.descriptor_attribute

So I was confused when my application failed to load code files

• I've defined attr for some of my classes
• and typing was defined for some entity is not being ready to instantiations
• which was loaded before inject.configure call
• so it failed — my app thrown traceback immediately after being launched

Then I found attr_dc which is closer to classic descriptor behavior. And also they behave differently (attr and attr_dc) which is confusing.
But it still lacks for some canonical descriptor behavior and rules of thumb.

I wanted to open this topic but @obs-gh-peterkolloch was the first one. And I joined here. 🙂

Summarizing:

1. can we remove context-manager support from attr?
2. can we limit attrs responsibility to manage instance values? (instances can share same value or have thread-local values or any other behavior under the hood — it depends on providers and binding configurations, not by descriptor itself)
3. can we make attr class field access (MyModelClass.descriptor_attribute) returning instance of attr, not trying to load object from configured binder?
4. can we leave only one descriptor to attach inject-based property to user classes? (get rid of attr_dc — I read original problem and new attr approach would cover it)

I would be glad to provide such PR if you ( @ivankorobkov ) would accept this general ideas and implementation logic.

Best regards, Dima

[pyctrl]

I think it would look like this:

class _AttributeInjection(Generic[T]):
    def __init__(self, cls: Binding) -> None:
        self._cls = cls

    def __get__(self, obj: Any, owner: Any) -> Injectable:
        if obj is None:
            return self

        inst = instance(self._cls)
        return inst

and work this way:

import inject
import dataclasses

@dataclasses.dataclass
class X:
    a = inject.attr(int)

inject.configure(lambda binder: binder.bind(int, 42))
# <inject.Injector object at 0x743b385a2510>

X.a
# <inject._AttributeInjection object at 0x743b385a23c0>

x = X()
x.a
# 42

[pyctrl]

Actually it is possible to support current behavior:

class _AttributeInjection(Generic[T]):
    def __init__(self, cls: Binding, inject_on_cls) -> None:
        self._cls = cls
        self._inject_on_cls = bool(inject_on_cls)

    def __get__(self, obj: Any, owner: Any) -> Injectable:
        if obj is None and self._inject_on_cls is False:
            return self

        inst = instance(self._cls)
        return inst

Here is usage:

import inject
import dataclasses

@dataclasses.dataclass
class X:
    a = inject.attr(int)
    b = inject.attr(int, inject_on_cls=True)

inject.configure(lambda binder: binder.bind(int, 42))


X.a
# <inject._AttributeInjection object at 0x7a47bb1f23c0>
X.b
# 42

X().a
# 42
X().b
# 42

[pyctrl]

Sorry for tons of letters. Just decided to describe not only in text but with real examples and code too.

[obs-gh-peterkolloch]

Thank you @pyctrl for all the thoughts and examples!

I agree with your thoughts.

Another concern is not to break currently working code.

A) I think that the partial context manager support (_GeneratorContextManager, why not AbstractContextManager?) for attribute injection was not really operational and removing it should not break things.

B) You thoughts about class attribute injection make sense to me. Not sure if someone depended on the current behavior, it can make sense and be useful.

A careful approach might expose the new behavior in new API functions.

Maybe inject.field could be used for this implementation?

#116 (comment)

That would also allow deprecating the other functionality (if wanted).

[obs-gh-peterkolloch]

It would be great to hear back from the original author @fhdufhdu and the reviewer/owner @ivankorobkov!

[ivankorobkov]

Hi!

I agree that context managers in attributes are not a good thing. There is no explicit scope to execute the context manager logic.

A) I think that the partial context manager support (_GeneratorContextManager, why not AbstractContextManager?) for attribute injection was not really operational and removing it should not break things.

Yes.

My perspective around attr-features:
they are Descriptors
Descriptors are commonly used to define some model fields (pydantic, marshmallow, Django, SQLAlchemy, etc...)

Yes, attr injects were always just simple descriptors.

class _AttributeInjection(object):
    def __init__(self, cls: Binding) -> None:
        self._cls = cls

    def __get__(self, obj: Any, owner: Any) -> Injectable:
        return instance(self._cls)

@pyctrl Sure, let's completely remove them from attributes. Please, make a PR, as you proposed above.

[obs-gh-peterkolloch]

@pyctrl Since you basically already wrote the code, it makes most sense that you create the PR! But feel free to reach out if you are out of bandwidth then I can do it.

[pyctrl]

Thanks for your support guys! I really appreciate it.

I published PR ^ — check it out. 👍

[ivankorobkov]

@obs-gh-peterkolloch I merged the PR. Could you please test that these changes resolve your issue? If everything's fine, I will make the next minor release.

[obs-gh-peterkolloch]

@ivankorobkov I dropped some comments. Furthermore, my code breaks due to other changes on master:

1. Incorrect autoparams signature, see https://mypy-play.net/?mypy=latest&python=3.12&gist=6537b99066d173f3190d0d9199501155 for a suggestion
2. Deleting attr_dc isn't friendly to existing users and a breaking change. 476bebf#r163127350 See https://typing.python.org/en/latest/spec/overload.html, the @overload functions should specify all possible type combinations.

[pyctrl]

@obs-gh-peterkolloch thank you! I agree with you. I will make PR covering your comments today a bit later. Really appreciate your help!