Page MenuHomeElementl

make build_resources usable as a function.
AbandonedPublic

Authored by cdecarolis on Apr 26 2021, 4:12 PM.

Details

Summary

Previously, build_resources could only be used as a context manager. This opens up build_resources to be used as a fxn as well, since most resources actually are not context managers.

Unfortunately, the implementation is a bit funky, as support for using something as both context manager and function in python has drawbacks. The gist is that to get context manager functionality, the returned object needs to be a context manager that can vend the required thing, and to be a function, the returned object basically needs to be the required thing. So I used the __getattr__ trick to vend attribute calls to the underlying constructed resources.

Test Plan

build_resources tests, added an additional test

Diff Detail

Repository
R1 dagster
Branch
build_resources_as_function
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 26 2021, 4:31 PM
Harbormaster failed remote builds in B29448: Diff 36150!
cdecarolis retitled this revision from build_resources as normal python fxn to make build_resources usable as a function..Apr 26 2021, 4:52 PM
cdecarolis added inline comments.
python_modules/dagster/dagster/core/execution/build_resources.py
42

I leave pipeline_run un-checked for mockability reasons

107

Not sure how to document this anymore. Do I call it a function, and then later on, say "hey this can also be used as a context manager"?

hmm this is a lot of trickery to have one API be able to do both things.

Its not clear to me that learning that this thing can be used as a context manager is better than learning there is a separate function ie managed_resources for the context managed case

if we go this path - we may want to run the tear down on __del__ if we didn't enter/exit

https://docs.python.org/3/library/contextlib.html#contextlib.ExitStack worth looking at

hmm this is a lot of trickery to have one API be able to do both things.

Its not clear to me that learning that this thing can be used as a context manager is better than learning there is a separate function ie managed_resources for the context managed case

That makes sense to me - but yea we are starting to run into this problem where the different possible API use cases each have ineficiencies making it necessary to increase surface area to support them:

  1. Most people using this API for testing would want to build a single resource and test it, hence build_resource.
  2. build_resource doesn't have an elegant solution to deal with resource to resource dependencies, hence build_resources.
  3. Resources can be context managers, and supporting a fxn to be both a context manager and a function is rough, so we need manage_resources
  4. Potentially manage_resource for the single-resource context manager case as well (dubious)

In summary, a lot of things that are possible, but rough to support all of them.

Does the standard Python library need to pull this kind of trickery for open?

Does the standard Python library need to pull this kind of trickery for open?

I believe it is implemented in a similar way - a function that returns a context manager. I think the big difference is that the thing returned by the fxn is the same as the thing returned by the cm: that is, the __enter__ method on the cm is just return self. We need to pull more shenanigans in order to get the nice resource context object in both cases.

can we add a test for using a context manager resource with the function API ? If we can determine at runtime that we got a CM back but we didn't enter a CM scope and error out - maybe thats a good end state for just having a flexible build_resources since the error message can put you on the right path

requesting to at least add a test case that demonstrates the experience - optionally make it a good experience

This revision now requires changes to proceed.Apr 27 2021, 10:10 PM

Update to check whether any resource is a context manager, and if it is, ensure that resources are being accessed in context.

There is still a possibility that we don't tear down properly here - if someone initializes a context manager but never tries to actually use anything from it, then we won't perform teardown. Trying to think of a place to inject teardown to ensure that it always occurs (if not in context manager, and not already torn down), I was thinking del?

Move teardown to __del__ if not in context manager scope. This ensures that teardown will always occur before the end of the program.

Thought of another issue: We only want to tear down an instance that we construct, not one that is passed in.

Ensure that we don't tear down a provided instance, only the one that we construct.

python_modules/dagster/dagster/core/execution/build_resources.py
103–107

maybe some comments here

python_modules/dagster/dagster_tests/core_tests/resource_tests/test_build_resources.py
112

you can force the python GC to run - gc.collect()

I think im cool with this once you can get the tests passing

This revision is now accepted and ready to land.May 4 2021, 8:06 PM

Add comments, simplify test