Paths

Table of Contentst

Differential D8053

Enable resource dependencies within memoizable io managers
ClosedPublic
Actions

Authored by • cdecarolis on May 24 2021, 4:56 PM.

Tags

None

Referenced Files

	Unknown Object (File)
	Dec 30 2022, 4:16 AM

	Unknown Object (File)
	Dec 19 2022, 8:03 AM

	Unknown Object (File)
	Dec 19 2022, 1:19 AM

	Unknown Object (File)
	Dec 8 2022, 11:04 AM

	Unknown Object (File)
	Dec 2 2022, 1:58 AM

	Unknown Object (File)
	Nov 28 2022, 12:57 PM

	Unknown Object (File)
	Nov 26 2022, 4:13 PM

	Unknown Object (File)
	Nov 15 2022, 11:16 AM

View All 68 Files

Subscribers

None

Details

Reviewers

• sandyryza
alangenfeld

Commits

R1:40a1c4d44b3e: Enable resource dependencies within memoizable io managers

Summary

Fix for https://github.com/dagster-io/dagster/issues/4204

Test Plan

Added test for resource dependencies.

Diff Detail

Repository

Branch

resources_output_context

Lint

Lint Passed

Unit

No Test Coverage

Event Timeline

• cdecarolis created this revision.May 24 2021, 4:56 PM

Harbormaster returned this revision to the author for changes because remote builds failed.May 24 2021, 5:15 PM

Harbormaster failed remote builds in B31055: Diff 38195!

Fix resources_for_io_manager callsites

Harbormaster completed remote builds in B31062: Diff 38202.May 24 2021, 6:11 PM

• cdecarolis requested review of this revision.May 24 2021, 6:11 PM

• sandyryza added inline comments.May 24 2021, 9:47 PM

python_modules/dagster/dagster/core/execution/context/output.py
201	In what situations would this be `None`?

• cdecarolis added inline comments.May 25 2021, 1:21 PM

python_modules/dagster/dagster/core/execution/context/output.py
201	Approximately `None`

• sandyryza added inline comments.May 25 2021, 3:31 PM

python_modules/dagster/dagster/core/execution/context/output.py
201	😂 In that case, can we make it non-optional? Then can avoid the `cast` below?

alangenfeld added inline comments.May 25 2021, 7:10 PM

python_modules/dagster/dagster/core/execution/context/output.py
201	yeah honestly if its not a user facing API i think we should avoid `= None` ing stuff out and just be explicit at all call sites. Too many bugs from accidental omission of optional arguments
201–202	passing the whole mode and scoped builder separately feels a bit odd to me maybe just take resource_defs directly instead of mode maybe just take the resources directly and only allow that if step_context is None?

Address comments: get rid of defaults on get_output_context, pass resources directly to get_output_context

Harbormaster completed remote builds in B31245: Diff 38448.May 26 2021, 3:50 PM

add test for pass both error

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

Closed by commit R1:40a1c4d44b3e: Enable resource dependencies within memoizable io managers (authored by Chris DeCarolis <cdguitar817@gmail.com>). · Explain WhyMay 26 2021, 6:46 PM

This revision was automatically updated to reflect the committed changes.

Chris DeCarolis <cdguitar817@gmail.com> added a commit: R1:40a1c4d44b3e: Enable resource dependencies within memoizable io managers.

Revision Contents
Changeset List

Path

Size

python_modules/

dagster/

dagster/

core/

execution/

context/

13 lines

2 lines

plan/

13 lines

resolve_versions.py

5 lines

dagster_tests/

core_tests/

storage_tests/

test_memoizable_io_manager.py

59 lines

Diff 38195

python_modules/dagster/dagster/core/execution/context/output.py

Loading...

python_modules/dagster/dagster/core/execution/context/system.py

Loading...

python_modules/dagster/dagster/core/execution/plan/utils.py

Loading...

python_modules/dagster/dagster/core/execution/resolve_versions.py

Loading...

python_modules/dagster/dagster_tests/core_tests/storage_tests/test_memoizable_io_manager.py

Loading...