-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
gh-141645: Add a TUI mode to the new tachyon profiler #141646
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
8cb430e to
8731b25
Compare
|
CC @lkollar |
|
@hugovk @AA-Turner @savannahostrowski any chance you could give this a go? Most of the diff is tests. You can focus on the main TUI code if you prefer. Even if you just test it locally it will help a lot! |
3922fb1 to
ab80fbf
Compare
lkollar
left a comment
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.
This is super cool! I've not reviewed the implementation thoroughly but I played around with the TUI and everything is working really well. Left a bunch of suggestions.
Co-authored-by: László Kiss Kollár <kiss.kollar.laszlo@gmail.com>
Co-authored-by: László Kiss Kollár <kiss.kollar.laszlo@gmail.com>
Co-authored-by: László Kiss Kollár <kiss.kollar.laszlo@gmail.com>
|
@lkollar i fix all your comments and clean the code a bit and added even more tests. I also added some color diffing to make it easier to spot changes. Can you give it another go? |
savannahostrowski
left a comment
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.
This is so slick 🔥 ! I played with this a bunch and the UX is very solid. I have one comment about an edge case I ran into and one suggestion (but it looks like you're already thinking about export so that's great)!
|
When you're done making the requested changes, leave the comment: |
|
@savannahostrowski I have addressed your comments and I pushed a new one ensuring the TUI (most of it) still works at the end in a way that makes sense. I left some comments regarding the exporting situation but we can surely explore that on a different PR :) Thanks a lot for the review ❤️ |
savannahostrowski
left a comment
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.
Tested out the fix for toggling help when profiling ends and it looks good!
Nice work on this one! Very, very cool! 🚀
This comment was marked as off-topic.
This comment was marked as off-topic.
|
test_profiling tests fail on buildbots installing Python. Example: https://buildbot.python.org/#/builders/207/builds/6809 cc @encukou |
I think we are missing something in the makefile will take a look |
Fixed in #141820 |
Uh oh!
There was an error while loading. Please reload this page.