Page MenuHomeElementl

make extra resources config optional
ClosedPublic

Authored by alangenfeld on Apr 9 2021, 7:33 PM.

Details

Summary

Make config for un-used resources optional

Test Plan

added test

Diff Detail

Repository
R1 dagster
Branch
extra-resources (branched from master)
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 9 2021, 7:52 PM
Harbormaster failed remote builds in B28663: Diff 35182!
Harbormaster returned this revision to the author for changes because remote builds failed.Apr 9 2021, 8:26 PM
Harbormaster failed remote builds in B28665: Diff 35185!
Harbormaster returned this revision to the author for changes because remote builds failed.Apr 9 2021, 9:11 PM
Harbormaster failed remote builds in B28666: Diff 35188!
Harbormaster returned this revision to the author for changes because remote builds failed.Apr 9 2021, 10:11 PM
Harbormaster failed remote builds in B28668: Diff 35190!

Very exciting.

Please consider my final comments especially around extracting resource requirements out of the validation step and having it be testable.

I'm also very interesting in seeing this change reflected in documentation and examples, but we can do that later. I'd love to get this out ASAP (esp before this week's release) so we can feedback from users (bob from goodeggs and Steve Pletcher come immediately to mind here). I think, similar to Sandy's typing change, we should announce this in #general.

I'll let the explicitly listed reviewers do the actual accept

python_modules/dagster/dagster/core/definitions/pipeline.py
31–32

comment-worthy

573–586

You might want to consider having resource_reqs determination outside of this function totally. This feels like "complecting" two separate operations.

I think a helper function that gathers all the required_resource_keys for a particular pipeline, mode tuple would be useful to have around that then you could compute the dictionary with a single for loop in its own function. This could also be tested independently

python_modules/dagster/dagster/core/definitions/pipeline.py
31–32

this is standard for mypy typing of things that are runtime circular deps [1]

573–586

yea the tricky bit is that as we traverse through the definition structure here we throw error messages that detail what is requiring the resource that is missing. I think the detailed error message are worth it, which makes refactoring for speculative re-use less clear. My best guess is its this shared traversal function with gathering & validating gated by some argument to allow for other uses.

823–829

[1]

I'd love to get this in for tmrw's release. Feel free to take stab at my proposed refactor or not. Your discretion.

This revision is now accepted and ready to land.Apr 14 2021, 4:59 PM

rebase, do some refactoring and leave useful comments explaning current setup

This revision was automatically updated to reflect the committed changes.