Skip to content

Add support for coroutine profiling - #255

Merged
fabianp merged 20 commits into
pythonprofilers:masterfrom
d-ryzhykau:async-profile
Jun 13, 2020
Merged

fabianp merged 20 commits into
pythonprofilers:masterfrom
d-ryzhykau:async-profile

Conversation

@d-ryzhykau

@d-ryzhykau d-ryzhykau commented Oct 25, 2019 •

Copy link
Copy Markdown
Contributor

This PR adds support for using @profile decorator on coroutines (both generator based coroutines and ones defined with async def statements).
Support for Python versions <3.4 is dropped.

@d-ryzhykau
d-ryzhykau marked this pull request as ready for review January 7, 2020 16:53
@d-ryzhykau
d-ryzhykau requested a review from fabianp February 4, 2020 15:28
@delijati

Copy link
Copy Markdown

can this be merged!

@fabianp

fabianp commented Apr 15, 2020

Copy link
Copy Markdown
Collaborator

Although I would like to see support for coroutines, I'm not comfortable with this PR changing the project organization, like distributing existing code over multiple files, as well as with the name of these files, like __main__.py

@d-ryzhykau

d-ryzhykau commented Apr 15, 2020 •

Copy link
Copy Markdown
Contributor Author

__main__.py makes memory profiler an executable package. And, IMHO, splitting the library into multiple small files would make maintenance easier.
@fabianp, if I manage to rewrite it keeping the memory_profiler.py untouched (and adding 1 extra file for asyncio support), would you be more comfortable with that?

@fabianp

fabianp commented Apr 15, 2020

Copy link
Copy Markdown
Collaborator

thanks for your answer @d-ryzhikov and apologies for my slow response.

This PR has two many changes in the code for me to review those changes and guarantee that it won't break other user cases, I hope you can understand that. Also, I would like to keep the code inside a single file (two if you count mprof) so that it can be more easily incorporated into other projects.

How many lines of code do you think adding support for coroutine profiling would add to memory_profiler.py if we were to keep the project with the current organization?

@d-ryzhykau

Copy link
Copy Markdown
Contributor Author

@fabianp , the async support itself won't be more than 40 lines. And it will have to be in a separate file if the support of Python version prior to 3.3 is necessary, because it will make use of yield from syntax (which was added in Python 3.3).

@fabianp

fabianp commented Apr 15, 2020

Copy link
Copy Markdown
Collaborator

I'm fine with coroutine support breaking for 3.3 as long as the other functionality still work in that version. Do you think this way it would be possible to have everything in the same file?

@d-ryzhykau

Copy link
Copy Markdown
Contributor Author

@fabianp , yes. If <3.3 support isn't neccessary, everything can be kept in 1 file.

@yoav-orca

Copy link
Copy Markdown

@fabianp @d-ryzhikov I would love to have this feature in, do you need help making the new PR?

Minor refactoring of `find_version` function.
Remove old python tags.
Revert package module list.
Specify required python version.
@d-ryzhykau

Copy link
Copy Markdown
Contributor Author

sorry 4 the wait
@fabianp, I reverted the 1-file structure of the code as requested dropping support of python <3.4 (including python2). I also did a little cleanup of the python2 specific code. Python>=3.4 is now required for the package (see this).

@fabianp
fabianp merged commit 68f2950 into pythonprofilers:master Jun 13, 2020
@fabianp

fabianp commented Jun 13, 2020

Copy link
Copy Markdown
Collaborator

Merged. Thanks a lot of the hard and continued work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants