Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide UndefVarError if the upstream cell errored #2057

Merged
merged 14 commits into from May 18, 2022

Conversation

HirumalPriyashan
Copy link
Contributor

@HirumalPriyashan HirumalPriyashan commented Apr 26, 2022

Implementation details

  • check whether the current is erred due to a UndefVarError.
  • find that undefined variable and check whether its due to an error in an upstream cell
  • if the err is due to an err in upstream cell, replace the current cell output with upstream cell's erred output

New behavior
image

@github-actions
Copy link
Contributor

Try this Pull Request!

Open Julia and type:

julia> import Pkg
julia> Pkg.activate(temp=true)
julia> Pkg.add(url="https://github.com/HirumalPriyashan/Pluto.jl", rev="fix-1179")
julia> using Pluto

@fonsp
Copy link
Owner

fonsp commented May 2, 2022

Do you think that it is possible to implement this in https://github.com/fonsp/Pluto.jl/blob/main/frontend/components/ErrorMessage.js instead? And always be sure to give a (short) description to you PR: how is it implemented, maybe explain why you chose this implementation, and show screenshots/videos of the new behaviour.

@HirumalPriyashan
Copy link
Contributor Author

@fonsp

Do you think that it is possible to implement this in https://github.com/fonsp/Pluto.jl/blob/main/frontend/components/ErrorMessage.js instead?

I have taken this approach as we don't have access to cell result of errored upstream cell (notebook.cell_results[upstream_cell_id]) in ErrorMessage.js

But seems like we can get those values through pluto_actions.get_notebook() in ErrorMessage.js. Will check the possibility of that approach and update the PR.

And always be sure to give a (short) description to you PR: how is it implemented, maybe explain why you chose this implementation, and show screenshots/videos of the new behaviour.

Updated the PR description.

@HirumalPriyashan
Copy link
Contributor Author

@fonsp

Updated the PR using https://github.com/fonsp/Pluto.jl/blob/main/frontend/components/ErrorMessage.js .
Since there is no point of showing upstream error body in the current cell, updated the msg with a new error msg.

New behavior
image

@fonsp
Copy link
Owner

fonsp commented May 10, 2022

Some ideas:

  • Can you make it recursive? i.e. if you have three cells, x = sqrt(-1), y = x, z = y, then the last two cells should show the redirect message instead of an undefvarerror.
  • What happens when there are multiple upstream variables? E.g. a = b = sqrt(-1), a + b.
  • Can you insert a clickable link to the upstream variable(s)?
  • Let's remove the stack trace

@HirumalPriyashan
Copy link
Contributor Author

  • Can you make it recursive? i.e. if you have three cells, x = sqrt(-1), y = x, z = y, then the last two cells should show the redirect message instead of an undefvarerror.
  • Let's remove the stack trace

At the moment, these two are already available with the changes. Please let me know if I did not understand it correctly. But I think we can point error in the third cell to a since the error is due to failure of computing of a not b.
image

  • What happens when there are multiple upstream variables? E.g. a = b = sqrt(-1), a + b.

Right now, it just displays the first upstream cell which identified as errored. But sure, we can update it for multiple variables.

  • Can you insert a clickable link to the upstream variable(s)?

Sounds cool, yes we can do that.

@HirumalPriyashan
Copy link
Contributor Author

HirumalPriyashan commented May 11, 2022

New behavior

  • Added a clickable link for upstream variables
  • Notice for the fifth cell errors are produced for the root cause, since c is errored due to a and b.

Edit:
Update after commit 8e0aaee

  • Removed the stacktrace

image

@fonsp
Copy link
Owner

fonsp commented May 18, 2022

This is great, thanks @HirumalPriyashan !!

I made some small changes, including:

  • dd0a631 (#2057): this is not really internal state (you don't want this value to persist across renders)
  • 1bb4aae (#2057) this fixes this issue, which could be confusing/frustrating in a large cell with multiple errors.

@fonsp fonsp merged commit fcd9b45 into fonsp:main May 18, 2022
@HirumalPriyashan
Copy link
Contributor Author

Awesome, thanks.

@fonsp fonsp linked an issue May 19, 2022 that may be closed by this pull request
@rikhuijzer rikhuijzer mentioned this pull request May 31, 2022
@KronosTheLate
Copy link

I am late to the party, but want to give a huge appreciation for this fix <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide UndefVarError if the upstream cell errored
3 participants