Page MenuHomePhabricator

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

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

Details

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
Branch
input_output_manager_consistency
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

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!
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!
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!
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!
python_modules/dagster/dagster/core/definitions/environment_configs.py
276

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?

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

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.

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

Is this still needed?

251–255

Manager key should never be none, right?

lgtm aside from my last couple comments

This revision is now accepted and ready to land.Jan 8 2021, 12:20 AM