-
Notifications
You must be signed in to change notification settings - Fork 433
Improve UX for memray attach and detach commands #841
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #841 +/- ##
==========================================
- Coverage 92.44% 91.83% -0.61%
==========================================
Files 99 99
Lines 11682 11768 +86
Branches 426 432 +6
==========================================
+ Hits 10799 10807 +8
- Misses 883 961 +78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
51665fa to
8313575
Compare
Users reported confusion when using memray attach with the --duration flag because the command would exit immediately without any indication that tracking had started successfully or was still running. This made it difficult to know if the attach operation worked, and if interrupted, there was no clear feedback about what would happen to the background tracking. Additionally, users would waste time going through the entire attach process only to discover at the end that their output file already existed, requiring them to restart with the --force flag. This change addresses these issues by checking the output file existence upfront before any injection work begins, and by showing detailed progress through each phase of the attach and detach operations. Users can now see exactly which step is in progress, making it easier to diagnose slow or stuck operations. For users who want the command to wait until tracking completes, a new --wait flag is provided that displays a live progress bar showing time elapsed and remaining. This makes the behavior more predictable while maintaining backward compatibility for users who expect the command to return immediately. Fixes bloomberg#831 Fixes bloomberg#701 Signed-off-by: Pablo Galindo <[email protected]>
8313575 to
fbd83af
Compare
| live_port = None | ||
| destination = memray.FileDestination( | ||
| path=os.path.abspath(args.output), | ||
| path=str(output_path), |
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.
The abspath that you removed was important; the running process might have a different working directory than the attaching process, so a relative path will go to the wrong spot.
|
Hm. I don't like I don't mind the progress indicators for the other operations, but it does seem to me that the complexity to value ratio is very high, and I wonder whether we can abstract some of that away. There's a ton of code added in this PR, but the user facing value is I think just barely higher than if we were to print out informative messages without the spinners. I think I might like this better without all of the |
Users reported confusion when using memray attach with the --duration flag
because the command would exit immediately without any indication that
tracking had started successfully or was still running. This made it
difficult to know if the attach operation worked, and if interrupted,
there was no clear feedback about what would happen to the background
tracking.
Additionally, users would waste time going through the entire attach
process only to discover at the end that their output file already existed,
requiring them to restart with the --force flag.
This change addresses these issues by checking the output file existence
upfront before any injection work begins, and by showing detailed progress
through each phase of the attach and detach operations. Users can now see
exactly which step is in progress, making it easier to diagnose slow or
stuck operations.
For users who want the command to wait until tracking completes, a new
--wait flag is provided that displays a live progress bar showing time
elapsed and remaining. This makes the behavior more predictable while
maintaining backward compatibility for users who expect the command to
return immediately.
Fixes #831
Fixes #701
Issue number of the reported bug or feature request: #
Describe your changes
A clear and concise description of the changes you have made.
Testing performed
Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.
Additional context
Add any other context about your contribution here.