search

bloomberg / memray

Memray is a memory profiler for Python
https://bloomberg.github.io/memray/
Apache License 2.0
13.36k stars 397 forks source link

import commands only when dependencies are available #606

Closed micheleAlberto closed 5 months ago

micheleAlberto commented 5 months ago

Issue number of the reported bug or feature request: #557

Describe your changes This PR allows the memray cli to run with a reduced set of dependencies. The cli will only list commands that are supported by installed packages and ignore the commands that require additional packages.

Testing performed without textual 

pip uninstall textual
memray --help

results in 

usage: memray [-h] [-v] [-V] {flamegraph,parse,stats,table,transform} ...

Memory profiler for Python applications

Run `memray run` to generate a memory profile report, then use a reporter command
such as `memray flamegraph` or `memray table` to convert the results into HTML.

Example:

    $ python3 -m memray flamegraph output.bin

positional arguments:
  {flamegraph,parse,stats,table,transform}
                        Mode of operation
    flamegraph          Generate an HTML flame graph for peak memory usage
    parse               Debug a results file by parsing and printing each record in it
    stats               Generate high level stats of the memory usage in the terminal
    table               Generate an HTML table with all records in the peak memory usage
    transform           Generate reports files in different formats

options:
  -h, --help            show this help message and exit
  -v, --verbose         Increase verbosity. Option is additive and can be specified up to 3 times
  -V, --version         Displays the current version of Memray

Please submit feedback, ideas, and bug reports by filing a new issue at
https://github.com/bloomberg/memray/issues

without jinja2

pip uninstall jinja2
memray --help

results in 

usage: memray [-h] [-v] [-V] {run,attach,live,parse,stats,summary,transform,tree} ...

Memory profiler for Python applications

Run `memray run` to generate a memory profile report, then use a reporter command
such as `memray flamegraph` or `memray table` to convert the results into HTML.

Example:

    $ python3 -m memray run -o output.bin my_script.py

positional arguments:
  {run,attach,live,parse,stats,summary,transform,tree}
                        Mode of operation
    run                 Run the specified application and track memory usage
    attach              Begin tracking allocations in an already-started process
    live                Remotely monitor allocations in a text-based interface
    parse               Debug a results file by parsing and printing each record in it
    stats               Generate high level stats of the memory usage in the terminal
    summary             Generate a terminal-based summary report of the functions that allocate most memory
    transform           Generate reports files in different formats
    tree                Generate a tree view in the terminal for peak memory usage

options:
  -h, --help            show this help message and exit
  -v, --verbose         Increase verbosity. Option is additive and can be specified up to 3 times
  -V, --version         Displays the current version of Memray

Please submit feedback, ideas, and bug reports by filing a new issue at
https://github.com/bloomberg/memray/issues

without jinja2 and textual

pip uninstall jinja2 textual
memray --help

results in 

usage: memray [-h] [-v] [-V] {parse,stats,transform} ...

Memory profiler for Python applications

Run `memray run` to generate a memory profile report, then use a reporter command
such as `memray flamegraph` or `memray table` to convert the results into HTML.

Example:

positional arguments:
  {parse,stats,transform}
                        Mode of operation
    parse               Debug a results file by parsing and printing each record in it
    stats               Generate high level stats of the memory usage in the terminal
    transform           Generate reports files in different formats

options:
  -h, --help            show this help message and exit
  -v, --verbose         Increase verbosity. Option is additive and can be specified up to 3 times
  -V, --version         Displays the current version of Memray

Please submit feedback, ideas, and bug reports by filing a new issue at
https://github.com/bloomberg/memray/issues

Additional context Add any other context about your contribution here.

godlygeek commented 5 months ago

@micheleAlberto Would you mind updating your commit to include a Signed-off-by? See https://github.com/bloomberg/memray/blob/main/CONTRIBUTING.md#contribution-licensing

Since this is multiple commits, the easiest way to do that would be using a git rebase -i against the main branch and then doing reword for each commit to add the Signed-off-by line.

codecov-commenter commented 5 months ago

Codecov Report

Attention: Patch coverage is 71.42857% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 92.96%. Comparing base (41248ed) to head (7ff82f7). Report is 79 commits behind head on main.

Files Patch % Lines
src/memray/commands/protocol.py 60.00% 4 Missing :warning:
src/memray/commands/run.py 81.81% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #606 +/- ## ========================================== + Coverage 92.55% 92.96% +0.41% ========================================== Files 91 95 +4 Lines 11304 11423 +119 Branches 1581 2092 +511 ========================================== + Hits 10462 10619 +157 + Misses 837 804 -33 + Partials 5 0 -5 ``` | [Flag](https://app.codecov.io/gh/bloomberg/memray/pull/606/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | Coverage Δ | | |---|---|---| | [cpp](https://app.codecov.io/gh/bloomberg/memray/pull/606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `92.96% <71.42%> (+7.02%)` | :arrow_up: | | [python_and_cython](https://app.codecov.io/gh/bloomberg/memray/pull/606/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `92.96% <71.42%> (-2.76%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

godlygeek commented 5 months ago

Thank you for the work you put in here, @micheleAlberto, but after some reflection, I've decided against going ahead with this approach.

The risk that this approach introduces is that, because the set of reporters to be exposed in any given environment is determined implicitly by whether they can be successfully imported, we might introduce a new dependency on a library that's not guaranteed to be installed, and accidentally trigger the removal of a reporter that we meant to include in our default set. This case isn't as far fetched as it might seem, and some of these dependencies are quite subtle. For instance, memray summary can't be imported unless Textual is installed. This isn't because it uses Textual, but rather because it imports a single function from memray.reporters.tui, to avoid duplication, which causes textual to be imported. The fix for that is factoring the function that needs to be shared into a common module that both memray.reporters.tui and memray.reporters.summary can depend upon, but just ignoring every module that can't be imported would lead to us not identifying problems like that, and potentially cause regressions if similar problems are introduced in the future.

Going further, another problem with making detection of optional libraries implicit rather than explicit is that we don't have a good way to notify the user of the missing dependency. I think what we want is for memray tree to be present in the --help and runnable from the CLI, but to emit a message if it's run explaining that it needs Textual to work, and explaining how to go about installing that dependency. But, emitting that message is only doable if we know what package to tell the user to install: we need to know that memray tree depends on Textual but memray flamegraph depends on Jinja2 to emit the right error message for each.

I do appreciate the work that you put in here, and it's certainly helped me solidify my understanding of the problem and what a solution needs to look like, but I don't think it's a good starting point to get to the best solution.

Sorry!