Page MenuHomeElementl

[depends on D7596] build_resource
AbandonedPublic

Authored by cdecarolis on Apr 26 2021, 5:24 PM.

Details

Summary

This diff adds a build_resource method on top of build_resources. Most users will only ever have to use the build_resources API(s) for testing the initialization and fxnality of their resources. For these users, it is annoying to have to boilerplate out the creation of a Resources context object, and then get the resource off of that object. This provides a nice interface for those use cases.

Test Plan

Added unit tests

Diff Detail

Repository
R1 dagster
Branch
build_resource
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, 5:41 PM
Harbormaster failed remote builds in B29477: Diff 36183!
cdecarolis retitled this revision from [build_resources improvements 2/n] build_resource to [depends on D7596] build_resource.Apr 26 2021, 5:59 PM

can we add a test for a context manager based resource? what happens? does it just silently not tear down ?

It makes sense to have nice APIs for the simple common case - but I think we should be intentional about what the behavior is when these get used with the complex rare case. I am suspect that just ignoring it is what we want

This discussion may be more appropriate to settle on the diff below

can we add a test for a context manager based resource? what happens? does it just silently not tear down ?

It makes sense to have nice APIs for the simple common case - but I think we should be intentional about what the behavior is when these get used with the complex rare case. I am suspect that just ignoring it is what we want

This discussion may be more appropriate to settle on the diff below

When you say ignoring, do you mean ignoring the cm-based resource? That makes sense to me, but then we need some way of determining that something is or isn't a context manager, which might be tricky.

When you say ignoring, do you mean ignoring the cm-based resource? That makes sense to me,

to clarify my position - I doubt that silently not running tear down for a CM resource is what we want to do

but then we need some way of determining that something is or isn't a context manager, which might be tricky.

we know
https://dagster.phacility.com/source/dagster/browse/master/python_modules/dagster/dagster/core/execution/resources_init.py$276-278
its just about threading the information around

Respin with better context manager error support, wait to see how build_resources changes land