Page MenuHomePhabricator

Remove DagstermillExecutionError altogether

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



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


Diff Detail

R1 dagster
removedagstermillexecution (branched from master)
Lint OK
No Unit Test Coverage

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?




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.Jan 4 2021, 4:23 PM
This revision was automatically updated to reflect the committed changes.