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

import commands only when dependencies are available #606

Closed

Conversation

micheleAlberto
Copy link
Contributor

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
Copy link
Contributor

@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.

This lays the groundwork for shipping a version of Memray
where these dependencies are optional.

Signed-off-by: Michele Alberto <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented May 21, 2024

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 ⚠️
src/memray/commands/run.py 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            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 Coverage Δ
cpp 92.96% <71.42%> (+7.02%) ⬆️
python_and_cython 92.96% <71.42%> (-2.76%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@godlygeek
Copy link
Contributor

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!

@godlygeek godlygeek closed this Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants