-
Notifications
You must be signed in to change notification settings - Fork 1k
Debug option for errors in visualisation #2747
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:
|
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.
Very cool @colinfrisch! I also have been frustrated by the lack of traceback with the visualization and adding the e
option is inspired, I would have just given them the traceback.
Hello @colinfrisch, thanks for this PR, great work! |
@Sahil-Chhoker thanks for the feeback ! I kind of liked the idea to have the option to keep a clean interface, especially if we use the terminal for other prints, but if everybody agrees, I'll just make the error plainly display :) |
I think he is suggesting to remove it, can you confirm @tpike3? Also have you tried testing it over on your model which you were having issues with? Because that is the best option to confirm its functionality. Also take a look at this article, if your haven't. |
I do like the idea of the 'e' option, but granted I have not used it. However, this is a fairly minor change and still an improvement, so always explore-exploit, so my bias would be to keep the 'e' and see how it plays out very easy to change. Either way good discussion @Sahil-Chhoker and @colinfrisch |
Thanks for your suggestions @Sahil-Chhoker and @tpike3 ! I haven't seen anything contradictory to this approach in the article that you sent me @Sahil-Chhoker, but I learned much. Did you think otherwise regarding the method ? I will be building a couple more models for the next weeks, so I'll be able to experiment fully the 'e' feature, I will give you guys feedback if you're interested ! Regarding the merge, I don't have the permission to make it. Are there more reviews required to implement it ? |
Looks good to me:) |
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 haven't seen anything contradictory to this approach in the article that you sent me @Sahil-Chhoker, but I learned much. Did you think otherwise regarding the method?
I just wanted you to read the alternatives, and I think the current implementation is just fine.
I found an issue about your "Press 'e'" feature, it blocks your current thread and therefore the error message on the frontend does not print but is just stuck as is.
Look at the image, here there is a mistake in agents.py
file but running the model does nothing, its stuck on step 0 and you won't know the error until you see the terminal and I don't like this behavior.
But that's just my opinion, I leave the final decision to you, @tpike3.
Just ran a test on this myself, you are right, when there is an error, your front end just freezes and you don't realize it is an error until you check the terminal.. that can be an issue ig |
Hmm that's a problem indeed... but wasn't solara already doing it even before this PR if an error happened during the simulation ? With just the full error, does it display it on the front end ? |
Solara frontend did indeed fail when we are just showing the error because again there is nothing blocking the main thread. Not sure about the full error, but in my opinion the full error is useful even if its just visible on the terminal.
You are free to do so if you want, just update or maybe open a follow-up PR if this PR gets merged. |
@Sahil-Chhoker @sanika-n I'm quite intrigued that you guys found the fact that solara freezes while there is an error specifically with this feature, because it happened to me every time that there was an error in the program... |
We have this same problem with |
Sorry for these manipulations... I accidentally closed the PR so I reopened it. By curiosity, what is left to do before the PR being merged ? |
Remove the |
Done ! |
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 am fine with these changes.
Great ! Then we just need the review from @Corvince and it's ready to merge ! |
Congrats on having your first PR merged! |
Resolves #2745
What the problem was
I noticed that when there is an error while visualising a mesa model with solara, it only displays the error type, not the classic debugging error (document, line, etc.).
For example, I got an error AttributeError: 'my_object' object has no attribute 'len', but I didn't know what part it was referring to, as I was using dependencies to different files that I created. It would be useful to have a possibility to see the full error for debugging.
How it fixes it
I only added an option to display the error if needed.
Now, instead of displaying
Error in step : [YOUR ERROR]
, it displaysError in step: [YOUR ERROR]. Press 'e' for full traceback
, if you press any other key, it doesn't show anything. It also does not prevent you from refreshing your visualisation or resetting it.