bloomberg / memray

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

Move the `memray attach` callback into a module #459

Closed godlygeek closed 10 months ago

godlygeek commented 1 year ago

Previously we were maintaining this as a string literal inside the client-side attach code, but it's growing more complex, and we're reaching the point where it would be advantageous to have it in a normal module that linters and type checkers can analyze.

This commit is based on the code in #455, and will need to be rebased once we land that, but it's ready for review other than that. Marking it draft until #455 is landed.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 80.28% and project coverage change: -0.11% :warning:

Comparison is base (bb9c0db) 91.88% compared to head (d47422d) 91.77%. Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #459 +/- ## ========================================== - Coverage 91.88% 91.77% -0.11% ========================================== Files 90 92 +2 Lines 10766 10822 +56 Branches 1473 1486 +13 ========================================== + Hits 9892 9932 +40 - Misses 872 888 +16 Partials 2 2 ``` | [Flag](https://app.codecov.io/gh/bloomberg/memray/pull/459/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/459/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `84.99% <ø> (-0.25%)` | :arrow_down: | | [python_and_cython](https://app.codecov.io/gh/bloomberg/memray/pull/459/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `95.23% <80.28%> (-0.07%)` | :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. | [Files Changed](https://app.codecov.io/gh/bloomberg/memray/pull/459?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | Coverage Δ | | |---|---|---| | [src/memray/\_attach\_callback.py](https://app.codecov.io/gh/bloomberg/memray/pull/459?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#diff-c3JjL21lbXJheS9fYXR0YWNoX2NhbGxiYWNrLnB5) | `22.22% <22.22%> (ø)` | | | [src/memray/commands/attach.py](https://app.codecov.io/gh/bloomberg/memray/pull/459?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#diff-c3JjL21lbXJheS9jb21tYW5kcy9hdHRhY2gucHk=) | `59.41% <100.00%> (+5.47%)` | :arrow_up: | | [tests/integration/test\_attach.py](https://app.codecov.io/gh/bloomberg/memray/pull/459?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#diff-dGVzdHMvaW50ZWdyYXRpb24vdGVzdF9hdHRhY2gucHk=) | `91.22% <100.00%> (+5.11%)` | :arrow_up: | | [tests/unit/test\_attach.py](https://app.codecov.io/gh/bloomberg/memray/pull/459?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#diff-dGVzdHMvdW5pdC90ZXN0X2F0dGFjaC5weQ==) | `100.00% <100.00%> (ø)` | | ... and [5 files with indirect coverage changes](https://app.codecov.io/gh/bloomberg/memray/pull/459/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg)

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

godlygeek commented 1 year ago

Actually... I've realized this will break using a new version memray to attach to a Python process where an older version of memray is installed. Now, that's not really something we've ever officially supported, but it is something that has always mostly worked - the live TUI wouldn't work if the client and server use a different version of our record format, but I believe attaching a tracker that outputs to a capture file has always worked, and that's something that our users might be relying on without us realizing it. That's a strong argument for sticking with having the client inject the code to be executed, rather than having the server import it from its installed copy of memray.

godlygeek commented 10 months ago

Let's not do this. It doesn't work as nicely as I thought it would.