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

Add CLI to plot memory monitors #5807

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

knelli2
Copy link
Contributor

@knelli2 knelli2 commented Feb 25, 2024

Proposed changes

Examples of output:

no_color.pdf

with_color.pdf

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@knelli2 knelli2 added the cli/pybindings Command line interface & Python bindings label Feb 25, 2024
@knelli2 knelli2 requested a review from nilsvu February 25, 2024 18:46
Comment on lines 55 to 56
PROPERTIES
TIMEOUT 40
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, the command itself takes a very long time to run. I wasn't able to determine why, even with spectre --profile. Any suggestions of where I'm going wrong are welcome.

@nilsdeppe
Copy link
Member

I'll let @nilsvu review the actual code, but I have a feature request: would it be easy to add a flag like --top-n 5 to show only the 5 most significant users? On the plots you showed it was tricky to figure out from the legend which were the 3 big ones shown.

@nilsvu
Copy link
Member

nilsvu commented Feb 26, 2024

Good idea, and/or how about a stacked area plot like this: https://python-graph-gallery.com/stacked-area-plot/
Then only show the legend entry for the big ones (above 5% or something like that)

@nilsdeppe
Copy link
Member

I'd be fine with adding that as an option. I do find it easier in the current style to read off how many GB something is using. I think the stacked plot makes relative comparisons nice, though. So maybe a CLI flag to enable the stacking? E.g. --stacked-plot?

@knelli2
Copy link
Contributor Author

knelli2 commented Feb 26, 2024

I'm personally not a big fan of the stacked plot. It's a bit ambiguous to me whether to interpret a stack as relative to zero, or relative to the stack below it. I do like the --top-n option though. I'll add that.

@nilsvu
Copy link
Member

nilsvu commented Feb 26, 2024

True. Maybe just combine the entries with only a couple percent into a single entry, then you don't need an extra CLI option and you can't see those small entries in the plot anyway. Also sort the entries.

@knelli2 knelli2 force-pushed the plot_mem_monitor branch 2 times, most recently from 1ce53eb to 6fd0ea5 Compare February 27, 2024 19:01
@knelli2
Copy link
Contributor Author

knelli2 commented Feb 27, 2024

Examples of output now with fixup

no_color

with_color

@nilsdeppe
Copy link
Member

LGTM, once @nilsvu is happy with the python code, please go ahead and merge. No need for my approval.

)
# Plotting options
@click.option(
"--use-mb/--use-gb",
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove this. Either make it always GB or switch to MB if the total memory is below 1 GB. This is just another option the person using this has to learn and set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to keep this one. I did make the default GB though since that's more likely

Copy link
Member

Choose a reason for hiding this comment

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

Ok

src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
Copy link
Member

@nilsvu nilsvu left a comment

Choose a reason for hiding this comment

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

Some simplifications with pandas. You can squash everything.

src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
ax.plot(
totals_df["Time"],
totals_df[component] / divisor,
linewidth=0.2,
Copy link
Member

Choose a reason for hiding this comment

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

Is the line width needed because the time sampling is so dense? Generally better to leave style up to the stylesheet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this so the total line would stand out in comparison to the other lines

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, maybe the color is enough? Up to you.

src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
src/Visualization/Python/PlotMemoryMonitors.py Outdated Show resolved Hide resolved
@nilsdeppe nilsdeppe merged commit f9655dd into sxs-collaboration:develop Mar 6, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli/pybindings Command line interface & Python bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants