improvement: strict mode NameError fix + cleaner exceptions #1829
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
marimo/_runtime/executor.py
Outdated
| (cell.last_expr, glbls) | ||
| ) | ||
| # fmt: on | ||
| except Exception as e: |
There was a problem hiding this comment.
This should be except BaseException as e:. Some libraries throw BaseExceptions (polars is one), and we need to communicate that exception info back to the user instead of hiding it in an "Unknown Error".
There was a problem hiding this comment.
Taking a step back, related to my comment above — instead of handling a special MarimoBaseException, would it be possible to just rewrite the traceback and hide marimo internals?
There was a problem hiding this comment.
Could just modify the traceback by throwing out all frames preceding the marimo cell's execution (ie lines before File ...__marimo__cell...)
There was a problem hiding this comment.
Good catch! This is the source of the failing test. Fixed. Re-stack trace manipulation, see other comment.
I think that MarimoBaseException is probably not correctly named. It's really the MarimoExecutionException or CellRuntimeException, and provides the context that it was a evaluation cell that failed. Being able to unwrap the exception for a cleaner stack trace is definitely a plus, but not the only positive since it's an indication the failure came from a cell failure.
LMK if this context changes your thoughts/ if you have thoughts for a better name
marimo/_runtime/executor.py
Outdated
| if _is_coroutine(cell.body): | ||
| # fmt: off | ||
| ( | ||
| await marimo_output_evaluation | ||
| (cell.body, glbls) | ||
| ) | ||
| # fmt: on | ||
| else: | ||
| # fmt: off | ||
| ( | ||
| marimo_body_execution | ||
| (cell.body, glbls) | ||
| ) | ||
| # fmt: on | ||
|
|
||
| if _is_coroutine(cell.last_expr): | ||
| # fmt: off | ||
| return ( | ||
| await marimo_output_evaluation | ||
| (cell.last_expr, glbls) | ||
| ) | ||
| # fmt: on | ||
| else: | ||
| # fmt: off | ||
| return ( | ||
| marimo_output_evaluation | ||
| (cell.last_expr, glbls) | ||
| ) | ||
| # fmt: on |
There was a problem hiding this comment.
Instead of formatting our source code to look good in a traceback, would it be possible to just modify the traceback in cell_runner to hide marimo internals? This seems difficult to maintain.
There was a problem hiding this comment.
I think modifying the stack trace, especially with different traces between script mode and notebook mode might introduce more code debt than this.
However, I do agree, that this is a little gimmicky (esp, since lint has to be manually disabled) and not needed.
The current trace of
return eval(cell.last_expr, glbls)
^^^^^^^^^^^^^^^^^^^^^^^^^^^Is fine, and I think it'd be better to revert to this than introduce stack trace manipulation.
An intermediate is without the formatting
return marimo_output_evaluation(cell.last_expr, glbls)But at that point, the eval makes more sense. LMK what you think
There was a problem hiding this comment.
Okay, makes sense. Reverting to eval/exec without special formatting sounds good to me.
|
Also added the name error catching for normal mode:
With other regular NameErrors passing through:
MarimoRef NameErrors in Strict Mode are a bit more verbose, since they should not occur:
with a potentially detected NameError preventing execution:
& edgecase for self definition
|
dmadisetti
changed the title
dmadisetti
changed the title
There was a problem hiding this comment.
Few more comments. I do think that wrapping exceptions in MarimoBaseException complicates things.
I've seen approaches for directly modifying tracebacks, e.g. discussed here: https://discuss.python.org/t/dropping-frames-from-a-traceback/10385. I know I asked before, but couldn't it be feasible to just omit frames that precede the first file with name *__marimo__cell_* in CellRunner? I guess I don't see how that introduces tech debt.
|
Not the most dramatic change, but I think it will pair very nicely with #1799 |
|
Reran and it's passing. Maybe just flaky. |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.7.12-dev6 |
📝 Summary
Previous Exceptions:
vs
Also clean up the name error for strict execution.
Your PR will be reviewed more quickly if you can figure out the right person to tag with @ -->
@akshayka OR @mscolnick