Page MenuHomeElementl

build_init_resource_context
ClosedPublic

Authored by cdecarolis on Wed, May 19, 9:10 PM.

Details

Summary

This diff adds a context builder for resource init context. This helps flesh out a testing story for resources.

Two side effects of this change:

  • InitResourceContext is no longer a namedtuple
Test Plan

unit + switching over existing tests

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Wed, May 19, 9:35 PM
Harbormaster failed remote builds in B30851: Diff 37945!

Add to test using resource_fn

Harbormaster returned this revision to the author for changes because remote builds failed.Thu, May 20, 2:45 PM
Harbormaster failed remote builds in B30883: Diff 37982!
python_modules/dagster/dagster/core/execution/context/init.py
44

I think we should keep around log_manager as a property, because it's cheap to include and might help someone avoid breakage.

149

I think it's worth being explicit and calling it build_init_resource_context. Otherwise might be confused with a context that includes a resource.

151

Having resource_def here is a little bit awkward. Some people might expect us to set this automatically. But that might be weird to do?

154

Docstring plz

python_modules/dagster/dagster/core/execution/context/init.py
44

fair enough

149

fine by me

151

if we do direct invocation magic instead of invoking the resource_fn we could foist it in

Keep around log_manager, build_resource_context -> build_resource_init_context, docstring

This revision is now accepted and ready to land.Fri, May 21, 5:30 PM

no wait im gonna put resource invocation in here too

This revision is now accepted and ready to land.Fri, May 21, 6:28 PM
cdecarolis requested review of this revision.

bump for review

This revision is now accepted and ready to land.Fri, May 21, 8:05 PM
python_modules/dagster/dagster/core/definitions/resource.py
163–173

these rules feel aggressively strict - why cant i kwarg my one thing in with whatever name i gave it?

Also what does "compute function" mean here? strange phrasing

python_modules/dagster/dagster/core/definitions/resource_invocation.py
80–89

same error message but different code - can these be more specific?

90–98

why are we blanking out the pipeline run on the context instead of passing through?

python_modules/dagster/dagster_tests/core_tests/resource_tests/test_resource_invocation.py
29

weird " "

72

weird ""

sandyryza retitled this revision from [RFC] build_resource_context to build_init_resource_context.Mon, May 24, 3:45 PM

@schrockn something to get your eyes on: this diff allows direct invocation of resources. I am comfortable with this. A little less comfortable than with solid though. A solid definition feels a lot like a function. A resource definition mostly feels like a function. I don't think direct invocation makes sense on all decorator-produced definitions. E.g. even though you can create a schedule definition by decorating a function, a schedule definition does not feel like a function.

Echoing Sandy's above sentiment - for now I'm thinking direct invocation will be the story only for solids and resources. We could optionally support others down the line if we wanted to, but I think it only truly makes sense for these two.

Make context available as kwarg, address comments

I think this is a fine incremental improvement. To me the biggest question (which this begins to address in some ways) is testing complex resources in a nice way. Would love to see how this feels on dogfooding/demo pipelines

python_modules/dagster/dagster_tests/core_tests/resource_tests/test_build_resource_context.py
47–49 ↗(On Diff #38185)

would be nice just for documentation/clarity to assert against an error message or message subset here

54 ↗(On Diff #38185)

I'm particularly interested in these types of use cases for testing intricate resources. @sandyryza really feeling your desire here to pass resource definitions or objects directly.

i.e.

with build_resource_init_context(resources={"foo": "foo"}) as context:

also working would be nice

python_modules/dagster/dagster_tests/core_tests/resource_tests/test_build_resource_context.py
22 ↗(On Diff #38185)

[1]

54 ↗(On Diff #38185)

@schrockn if I understand correctly, then we do actually support this. See [1]

python_modules/dagster/dagster_tests/core_tests/resource_tests/test_build_resource_context.py
47–49 ↗(On Diff #38185)

will do

Will add a follow up diff to this to migrate over some demo tests

python_modules/dagster/dagster_tests/core_tests/resource_tests/test_build_resource_context.py
54 ↗(On Diff #38185)

oh awesome!

One thing I missed here: can we do build_init_resource_context to match the word ordering in InitResourceContext?

This revision now requires changes to proceed.Wed, May 26, 7:51 PM

Will add a follow up diff to this to migrate over some demo tests

One thing I missed here: can we do build_init_resource_context to match the word ordering in InitResourceContext?

ha yes I just fixed this on local rev. Nice catch.

This revision is now accepted and ready to land.Thu, May 27, 3:03 PM

align name with InitResourceContext, spin through some tests on internal, fix default config

cdecarolis edited the summary of this revision. (Show Details)

respun using new invocation strategy, renamed context builder, fixed config issues

This revision is now accepted and ready to land.Thu, Jun 3, 4:03 PM
python_modules/dagster/dagster_tests/core_tests/resource_tests/test_resource_invocation.py
71

Is there a way we can get this lint warning to go away? Ideally users shouldn't need to disable pylint to test a context manager resource.

Spitballing, could we enable a user to do:

@resource
@contextmanager
def cm_resource(_):
    try:
        yield "foo"
    finally:
        teardown_log.append("collected")
python_modules/dagster/dagster_tests/core_tests/resource_tests/test_resource_invocation.py
71

Hm. I dont think I see how this would solve the issue. We can already determine whether or not the underlying resource_fn is a generator, I think the lint error is occurring because pylint can't determine that the return of call is potentially a cm.

python_modules/dagster/dagster_tests/core_tests/resource_tests/test_resource_invocation.py
71

adding to https://dagster.phacility.com/source/dagster/browse/master/.pylintrc$24 should help, at least for us internally

python_modules/dagster/dagster_tests/core_tests/resource_tests/test_resource_invocation.py
71

I don't think pylint is smart enough to look at the relevant call. I think it just looks at what's decorated.

python_modules/dagster/dagster_tests/core_tests/resource_tests/test_resource_invocation.py
71

to clarify adding resource to that list should cause pylint to make less assertions about`@resource` decorated functions, since it has been told the decorator mutates the original function signature in some way.

I'm not certain it will fix this context manager case, but I think it might

python_modules/dagster/dagster_tests/core_tests/resource_tests/test_resource_invocation.py
71

does not fix context manager error unfortunately

Making resource decorate a cm fixes lint issue