Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase granularity of context. #98

Open
alexanderlazarev0 opened this issue Oct 5, 2024 · 9 comments
Open

Increase granularity of context. #98

alexanderlazarev0 opened this issue Oct 5, 2024 · 9 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@alexanderlazarev0
Copy link
Collaborator

Currently, container_context() resets/reinitializes the whole context. This makes it hard to manage resources with different lifetimes.
As a theoretical workaround, it is possible to build an initial_context that keeps resources in the scope, but that requires using context internals (starting with _).

Proposal:

  • Allow passing providers to container_context:
async with container_context(container.provider1, container.provider2, ...): 
# resets the context for listed providers.
  • Allow passing containers to container_context:
async with container_context(container1, container2, ...):
# resets the context for context-resources in given containers

Implementation details:

  • Make each ContextResource manage its own context(var) on a class level.
  • container_context now just interacts with these context(var)s to set and reset.

Goals:

  • Maintain compatibility with the current solution:
async with container_context(): # will still reset all contexts across all containers
  • Maintain current functionality with initial_context

@lesnik512 If you approve of this proposal, I will start working on it. I also saw that you make active use of setting the initial_context, if you could provide a functional example I will make sure to test against it when converting functionality.

@alexanderlazarev0 alexanderlazarev0 self-assigned this Oct 5, 2024
@alexanderlazarev0 alexanderlazarev0 added enhancement New feature or request question Further information is requested labels Oct 5, 2024
@lesnik512
Copy link
Member

@alexanderlazarev0 seems promising!

Make each ContextResource manage its own context(var) on a class level.

Do you mean that in init method of ContextResource will be created context var and context stack will be stored in it?

I approve. And I will provide an example of initial context usage

@alexanderlazarev0
Copy link
Collaborator Author

Do you mean that in init method of ContextResource will be created context var and context stack will be stored in it?

@lesnik512 Yes (this was a mistake of mine in the post), since each ContextResource instance would need to manage its own context. What is should have said is at instance level.

@alexanderlazarev0 alexanderlazarev0 removed the question Further information is requested label Oct 5, 2024
@lesnik512
Copy link
Member

@alexanderlazarev0 this could work, thank you for clarification

@lesnik512
Copy link
Member

@alexanderlazarev0 I thought about separate context vars.

Do we really need this? I think we can achieve described behaviour with single contextvar

@lesnik512
Copy link
Member

@alexanderlazarev0 about example of initial context usage: this example is quite good https://github.com/modern-python/that-depends/blob/main/tests/integrations/test_fastapi_di_pass_request.py

@alexanderlazarev0
Copy link
Collaborator Author

alexanderlazarev0 commented Oct 8, 2024

@lesnik512

Do we really need this? I think we can achieve described behaviour with single contextvar

Currently, all context resources are managed using one context-variable that stores a dictionary. In practice, this dictionary has a shared namespace (dict-keys) for both:

  1. explicitly set context-resources (as in the example you provided in the previous comment):
async with container_context({"my_custom_key" : some_object }): ...

... that can then be retrieved anywhere within that context using:

some_object = fetch_context_item("my_custom_key")
  1. context-resources that are created by ContextResource providers, whose data is also stored in the same dict but contains a uuid in the key to avoid collision. These keys also make these resources hard to fetch using fetch_context_item and hard to set a pre-determined value for using initial_context, since retrieving the key requires some kind of "workaround".

Also it is important to note that entering a new container context using:

async with container_context()

Resets our dict completely without preserving any user set context-resources and provided context-resources.

From this I think we have a clear separation of context-resources which I think we should handle differently.

  1. We continue handling user set context-resources in a "global" dict, however, we introduce an additional argument into container_context:
async with container_context(preserve_globals=true): ...

Where preserve_globals (we can rework the name) is an optional kwarg that has true (? not sure whats best here) default. This will make it such that entering a new container_context does not reset the dictionary and preserves objects that were set with initial_context or otherwise.

  1. We start handling ContextResource provided resources using ContextResource instance level contextvars which are specific to that resource. This allows us to create a clear separation between these resources and separate management of these resources to another location. Yes, in theory everything still could be managed in one dict, but I think that will become more difficult to manage in the future.

Finally, the API would look something like this:

@container_context() # resets all context-providers, but global context is preserved
async foo(resource = Provide[MyContainer.context_resource]): ...

@container_context(initial_context=my_custom_context) # set new global context to my_custom_context and reset all providers.
async foo(resource = Provide[MyContainer.context_resource]): ...

@container_context(preserve_globals=false) # completely clean context
async foo(resource = Provide[MyContainer.context_resource]): ...

@container_context(providers=[MyContainer.context_resource]) # reset only the given provider, keep global context.
async foo(resource = Provide[MyContainer.context_resource]): ...

@container_context(containers=[MyContainer]) # reset all context-resources in given container, keep global context.
async foo(resource = Provide[MyContainer.context_resource]): ...

@container_context(providers=[MyContainer.context_resource], preserve_globals=false) # reset given provider and global context.
async foo(resource = Provide[MyContainer.context_resource]): ...

And (for completeness):

  1. Resetting global context always works whether using async with container_context() or with_container_context().
  2. Using async with container_context() allows resetting all providers and containers (since we can call sync from async).
  3. Using with container_context (equivalent to wrapping a sync function with @container_context) allows only to reset sync providers (specifying async provider will throw an error), and if a container is specified, then only its sync ContextResource providers will be reset, async providers will keep their state. This is not an issue since once a sync container_context is entered, resolution of async dependencies is no longer possible (you are either already in a sync function OR you wrote the following code:
async def foo():
    with container_context(containers=[MyContainer]):
        await MyContainer.async_resource.async_resolve()  # Error: Cannot resolve async resource in sync container_context()

So we should specifically watch out for this case (and also mark it bad practice to enter with container_context() inside an async function.

Please let me know what you think)

@lesnik512
Copy link
Member

@alexanderlazarev0 I agree that using separate contextvars can be easier to manage, but I have several concerns/questions:

  1. how without passing providers and containers to container_context to tear down all context resources? Do you want to to use subclasses() method of BaseContainer?
  2. where we will store initial context? Will it be separate contextvar?
  3. I'm concerned about performance degradation. What do you think, will it significantly affect performance?

@alexanderlazarev0
Copy link
Collaborator Author

alexanderlazarev0 commented Oct 9, 2024

@lesnik512

how without passing providers and containers to container_context to tear down all context resources? Do you want to to use subclasses() method of BaseContainer?

My initial approach would be to modify the BaseContainer or its meta to "register" all containers in a class variable, likely also a contextvar.

where we will store initial context? Will it be separate contextvar?

Yes initial_context and all the variables which have user set key can go in a separate contextvar

I'm concerned about performance degradation. What do you think, will it significantly affect performance?
I see two reasonable approaches:

  1. Have two contextvars: one for user-set resources, and one for providers
  • fast reinit of context when all providers need to be reset, just contextvar.set({}}
  • Each time I want to reinit the context of only 1 provider, I still need to copy the rest of the "context-dict" to the new context O(numOfContextProviders*averageContextStackDepth) space complexity.
  • Each time the context of a whole container needs to be reset I still have to traverse all the providers, get their keys and then remove them from context.
  1. Have one contextvar for user-set resources, and have all providers manage their own contextvars
  • need to traverse all providers when fully resetting context and performance an isinstance check to then reinit their context O(numberOfProviders) time complexity.
  • Each time I want to reinit the context of only 1 provider, I don't need to copy the context of the other providers along with it.
  • Resetting on container level is exactly the same.

So there is a trade of time-complexity and space-complexity.

@lesnik512
Copy link
Member

@alexanderlazarev0 ok, let's make separate contextvars then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants