Page MenuHomePhabricator

Remove DagstermillExecutionError altogether
ClosedPublic

Authored by dgibson on Dec 22 2020, 6:25 PM.

Details

Summary

This used to be inside a user_error_code_boundary, and I left it in a weird state where it was kind of a user code error boundary and kind of not.

The thing is, this code is already covered by the step execution user error code boundary since it's inside a solid execution function, so we don't need another one. Can we just remove the redundant boundary altogether? I don't see anything catching this specific error, and it just gets discarded and replaced with its underlying PapermillExecutionError (the user code exception) when an exception is bubbled up.

Test Plan

BK

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

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 22 2020, 6:39 PM
Harbormaster failed remote builds in B23339: Diff 28387!

I am pretty sure this is legit given how a lot of the dagstermill impl predates so much of the shifting that has happened around the core execution pathway but I will defer to @max to be sure

Can we see tracebacks before and after this change?

sure!

Before https://dagster.phacility.com/P141

After https://dagster.phacility.com/P140

Same exception type - the top and bottom of the stack traces are the same, the middle is slightly different now

This is fine with me as more consistent, the longer traceback is probably "worse" but this is a weird code path we shouldn't maintain.

This revision is now accepted and ready to land.Mon, Jan 4, 4:23 PM
This revision was automatically updated to reflect the committed changes.