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

utl: add Progress reporting + gui implementation #6792

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

gadfort
Copy link
Collaborator

@gadfort gadfort commented Feb 27, 2025

Adds:

  • Progress class to allow tools to announce their progress.
  • ProgressReporter individual class used to track progress of a specific tool/process (this allow us to have multiple reporters in parallel when/if that occurs).
  • Adds a reporter to DRT's main loop to allow us to exit early if an interrupt is received.
  • Adds a capture of control+C to get the interrupts and (3x control+C in less than 10 seconds will terminate openroad).

Notes:

  • We can go over the API to ensure it's contains what we want, but this was the minimum I found I needed to get the GUI working and capture interrupts.

GUI:
Adds a progress bar to the bottom right corner (this can display the progress of a single or multiple reporterts)
Pasted image
For example:
Pasted image (2)

Detailed view of reporters (this uses a set of reports that were interested for testing, but should not be included here):
Pasted image (3)

GUI interrupt:
Pasted image (4)

CLI:
Since there wasn't an easy tool to add reporting to, the logger is not getting exercised at the moment.

Signed-off-by: Peter Gadfort <peter.gadfort@gmail.com>
Signed-off-by: Peter Gadfort <peter.gadfort@gmail.com>
@gadfort gadfort requested a review from maliberty February 27, 2025 19:48
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Signed-off-by: Peter Gadfort <peter.gadfort@gmail.com>
Signed-off-by: Peter Gadfort <peter.gadfort@gmail.com>
Signed-off-by: Peter Gadfort <peter.gadfort@gmail.com>
Signed-off-by: Peter Gadfort <peter.gadfort@gmail.com>
@oharboe
Copy link
Collaborator

oharboe commented Feb 28, 2025

Nice! Some thoughts on use-case and user-experience.

I frequently run make GUI_TIMING=0 gui_xxx. However, this requires me to first start the GUI, discover that reading timing is taking too long, kill OpenROAD, restart with GUI_TIMING=0.

Ideally, I'd just launch the GUI and click "Skip" if something takes too long.

"Abort" or "Stop" sounds scary. I don't know what to expect of the state of the system is if I click "stop" or "abort". Sounds like the GUI is telling me: "If you click on the button, then you asked for it and you deserve the result".

However, if the GUI says "loading timing information" and there is a friendly "skip", then my expectation is that nothing scary will happen, it will be equivalent of make GUI_TIMING=0 gui_xxx

@gadfort
Copy link
Collaborator Author

gadfort commented Feb 28, 2025

@oharboe the messages can be adjusted, at the moment your usecase isn't supported anyways, because this will require some interface with opensta. The tools can end with a end(false) to indicate that no database changes were performed so the DB is safe, but if the tool is aborted and made changes, the database may no longer be in a safe state beyond inspection.
I think in all cases, I called it interrupt and not abort. Once the basic functionality / API is agreed upon we can revisit the messaging.
One usecase that is not covered here, but might be nice to have is a tool that cannot be interrupted, but still wants to provide a progress report (there the interrupt button should probably be disabled since it's not useful).

Signed-off-by: Peter Gadfort <peter.gadfort@gmail.com>
@maliberty
Copy link
Member

I tried it out and one thing I noticed is that when interrupting detailed_route the command still finishes with success and the remainder to the script continues on. I think if a command is interrupted we should stop further script execution as it is unlikely you want to subsequent commands to work on the incomplete result.

@maliberty
Copy link
Member

I was thinking that
image
would become the cancel button rather than adding another button for that purpose.

@maliberty
Copy link
Member

It feel redundant to have the iteration in the progress and status areas
image

@gadfort
Copy link
Collaborator Author

gadfort commented Mar 3, 2025

I tried it out and one thing I noticed is that when interrupting detailed_route the command still finishes with success and the remainder to the script continues on. I think if a command is interrupted we should stop further script execution as it is unlikely you want to subsequent commands to work on the incomplete result.

I can change it so end() emits an exception instead of a warning.

I was thinking that image would become the cancel button rather than adding another button for that purpose.

I'm fine implementing this as well, since we handle this button differently during a pause I was delaying implementing this till everything else looks alright.

It feel redundant to have the iteration in the progress and status areas image

I agree, this was left over from before the progress bar was there (easy to remove)

gadfort added 3 commits March 2, 2025 20:51
Signed-off-by: Peter Gadfort <peter.gadfort@gmail.com>
Signed-off-by: Peter Gadfort <peter.gadfort@gmail.com>
@gadfort
Copy link
Collaborator Author

gadfort commented Mar 3, 2025

@maliberty I enabled the run button to handle interrupts too, changed the end function to return true if an interrupt occurred and generate an error if there was an interrupt, and removed the status bar messages.

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.

3 participants