Page MenuHomeElementl

Move solid error boundary closer to user code invocation to clean up stack traces
ClosedPublic

Authored by owen on Jun 3 2021, 5:50 PM.

Details

Summary

Removes a couple of potentially confusing stack frames. I contemplated moving this even further (down into the solid compute_fn), but that seemed like it might be a bit too spicy.

Old Stack Trace:

Screen Shot 2021-06-03 at 10.37.53 AM.png (759×1 px, 240 KB)

New Stack Trace (if error before first event in the generator):

Screen Shot 2021-06-03 at 10.47.39 AM.png (734×1 px, 227 KB)

New Stack Trace (if error after first event in the generator):

Screen Shot 2021-06-03 at 10.54.39 AM.png (728×1 px, 213 KB)

Test Plan

bk

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 3 2021, 6:12 PM
Harbormaster failed remote builds in B31609: Diff 38912!
owen requested review of this revision.Jun 3 2021, 7:28 PM
python_modules/dagster/dagster/core/execution/plan/compute.py
104

What was the prior behavior? Were we failing to wrap these exceptions?

python_modules/dagster/dagster/core/execution/plan/execute_step.py
587

This function just a plain passthrough now, right? Can we get rid of it?

python_modules/dagster/dagster/core/execution/plan/compute.py
104

previously, this entire function was within an iterate_with_context() thing, so all exceptions were properly wrapped. however, I can't just wrap the entire body of this function in iterate_with_context() because you need to pass in the generator that you get from compute_fn() into iterate_with_context(), which you can't get until you execute some user code.

I could get around this by bumping the boundary out a level, but that would add another frame to the trace.

python_modules/dagster/dagster/core/execution/plan/execute_step.py
587

makes sense

alangenfeld added inline comments.
python_modules/dagster/dagster/core/execution/plan/compute.py
104

i'm confused a bit by this comment, the generator function doesn't evaluate until it starts getting iterated over from what i understand

(test) ~/dagster:master$ python
Python 3.8.7 (default, Feb 16 2021, 10:05:48)
[Clang 12.0.0 (clang-1200.0.32.29)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> def what():
...     print('ok')
...     yield 1
...     print('sure')
...     yield 2
...
>>> x = what()
>>> [i for i in x]
ok
sure
[1, 2]
python_modules/dagster/dagster/core/execution/plan/compute.py
104

@alangenfeld Yeah the comment is incorrect -- I was seeing some very unintuitive things, on account of how we coerce things into generators.

In master right now, we first execute the solid compute function (regardless of if it is a regular function or a generator), and then inspect the return value to determine if what we just executed was a generator or not. If it was a regular function, only now do we construct a generator to wrap this result object. This means that errors can surface before you start iterating through your generator (because the error-raising computation happened before we wrapped the result object in a function).

I feel like this is unnecessarily inconsistent behavior (and who knows, it might cause other weird and spooky things in other places), so I tried my hand at changing this flow so that you will never start executing solid code before you start consuming from the iterator.

I understand this is a bit of a spicy change, but I thought I would at least put it out there.

I understand this is a bit of a spicy change, but I thought I would at least put it out there.

I'm down, maybe lets just add a few new test cases since it is spicy? I believe the existing coverage is decent but the more the merrier

added some tests for weird things people might do with generators and solids

This revision is now accepted and ready to land.Jun 8 2021, 8:13 PM