-
Notifications
You must be signed in to change notification settings - Fork 1k
Debug option for errors in visualisation front end #2753
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
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Performance benchmarks:
|
Really cool! Just tried it out |
Thanks! @colinfrisch, could you rebase your branch on our @sanika-n is this otherwise good to merge (ideally use the review tool to comment/approve) |
Nice initiative, @colinfrisch! I'm not opposing your feature by saying this—just sharing a personal preference. I actually like it when my frontend breaks and shows the error directly; it's easier for me to see the full traceback on the screen rather than navigating through the terminal. That said, this is just my opinion, and I’ll leave the review and decision to @sanika-n. |
Alright, I'll do this asap ! |
@Sahil-Chhoker, I don't think that this PR prevents the simulation from breaking down and displaying the error on the full screen if there's an error in the build. It's tackling rather the simulation part once everything is built. Right now, if the simulation starts running correctly and an error comes up during the simulation, everything just freezes and, after the first PR, an error is displayed in the terminal. This current PR simply notifies the user that there is an error instead of just freezing and waiting. However, if you've tested a case where there should be a full breakdown and it doesn't happen anymore with this new code, I'd be interested to know about it :) |
No, I think I get it, sorry I was referring to something else, you're good. |
This is great stuff. @colinfrisch can you resolve the merge conflicts? I'll try to review asap afterwards. |
@quaquel @EwoutH, rebase and conflicts should be handled. Don't hesitate to let me know if you see any other thing I should do :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this PR, it's an improvement and the code is also minimal, I am approving the changes and leave the merge to @quaquel.
One little thing though, maybe add a comment after traceback.print_exc()
about what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff, will give Jan some time to review (and merge).
This is a followup of #2747 (merged)
What the problem was
I noticed that when there is an error while visualising a mesa model with solara, it does not display the error in the front-end.
After my first PR, the error now displays only in the terminal, but the user can still have a bit of trouble figuring out what is happening if everything just freezes.
How it fixes it
Uses
solara.use_reactive
to store the short version of the error as a reactive state in the simulation.I then added the display of the condensed error in the front end.
The first PR was about getting the full error in the terminal. However, after a few tests, I think that the "condensed" error is enough for the front end, in order to maintain clarity in the interface :
Test error on the foraging ants model I built : to reproduce this exact error, clone the ABM_mesa_models repo, and uncomment line 141 in
agents.py
Features
Thanks again to @EwoutH, @tpike3, @Sahil-Chhoker, and @sanika-n for the feedback in the previous PR, I'd love to hear your thoughts on this follow-up.