Page MenuHomePhabricator
Differential D5849

[I/o manager cleanup 2/2] make error states consistent between input and output managers
ClosedPublic
Actions

Authored by cdecarolis on Jan 6 2021, 8:17 PM.

Details

Reviewers
sandyryza
yuhan
Commits
R1:59d00bf86a77: [I/o manager cleanup 2/2] make error states consistent between input and output…
Summary

In reference to issue https://github.com/dagster-io/dagster/issues/3492.
This diff makes the error states consistent between input and output managers when [1] a resource is provided for a manager key that is not a manager [2] no resource is provided for a manager key.

Test Plan

Added unit tests for failure states

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cdecarolis created this revision.Jan 6 2021, 8:17 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 6 2021, 8:33 PM
Harbormaster failed remote builds in B23767: Diff 28903!
cdecarolis updated this revision to Diff 28911.Jan 6 2021, 9:28 PM

Up

cdecarolis updated this revision to Diff 28912.Jan 6 2021, 9:31 PM

Up

Harbormaster failed remote builds in B23774: Diff 28911!Jan 6 2021, 9:43 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 6 2021, 9:47 PM
Harbormaster failed remote builds in B23775: Diff 28912!
cdecarolis updated this revision to Diff 28917.Jan 6 2021, 10:08 PM

Up

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 6 2021, 10:27 PM
Harbormaster failed remote builds in B23779: Diff 28917!
cdecarolis updated this revision to Diff 28950.Jan 7 2021, 3:47 PM

Up

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 7 2021, 4:05 PM
Harbormaster failed remote builds in B23811: Diff 28950!
cdecarolis updated this revision to Diff 28951.Jan 7 2021, 4:20 PM

Up

Harbormaster completed remote builds in B23812: Diff 28951.Jan 7 2021, 4:39 PM
cdecarolis requested review of this revision.Jan 7 2021, 4:39 PM
cdecarolis edited the summary of this revision. (Show Details)Jan 7 2021, 4:59 PM
sandyryza added inline comments.Jan 7 2021, 6:59 PM
python_modules/dagster/dagster/core/definitions/environment_configs.py
272

I believe the mode should be providing the resource def in all cases, even with the default key.

https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/core/definitions/mode.py#L59

Does that not match what you've observed?

cdecarolis added inline comments.Jan 7 2021, 9:22 PM
python_modules/dagster/dagster/core/definitions/environment_configs.py
272

ah i remember that this is the case. Oddly it does not match what I've seen. For example, in test_composite_descent.py some are getting to this stage with no resources. Looks like a bug, will investigate further. Thanks for catching.

cdecarolis updated this revision to Diff 29014.Jan 7 2021, 10:13 PM

Up

Harbormaster completed remote builds in B23864: Diff 29014.Jan 7 2021, 10:32 PM
sandyryza added inline comments.Jan 7 2021, 11:35 PM
python_modules/dagster/dagster/core/definitions/environment_configs.py
24

Is this still needed?

249–251

Manager key should never be none, right?

sandyryza accepted this revision.Jan 8 2021, 12:20 AM

lgtm aside from my last couple comments

This revision is now accepted and ready to land.Jan 8 2021, 12:20 AM
cdecarolis updated this revision to Diff 29075.Jan 8 2021, 3:51 PM

Up

Harbormaster completed remote builds in B23916: Diff 29075.Jan 8 2021, 4:09 PM
Closed by commit R1:59d00bf86a77: [I/o manager cleanup 2/2] make error states consistent between input and output… (authored by Chris DeCarolis <cdguitar817@gmail.com>). · Explain WhyJan 8 2021, 4:24 PM
This revision was automatically updated to reflect the committed changes.
Chris DeCarolis <cdguitar817@gmail.com> added a commit: R1:59d00bf86a77: [I/o manager cleanup 2/2] make error states consistent between input and output….

Revision Contents
Changeset List

PathSize
python_modules/
dagster/
dagster/
core/
definitions/
20 lines
storage/
8 lines
dagster_tests/
core_tests/
config_types_tests/
evaluator_tests/
25 lines
storage_tests/
51 lines
40 lines
31 lines

Diff 29079

View Options

python_modules/dagster/dagster/core/definitions/environment_configs.py

Loading...
View Options

python_modules/dagster/dagster/core/storage/memoizable_object_manager.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/config_types_tests/evaluator_tests/test_composite_descent.py

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/test_versioned_execution_plan.py

Loading...