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

WIP: Independent child process monitoring #118 #134

Merged
merged 9 commits into from
Mar 21, 2017

Conversation

bbengfort
Copy link
Contributor

This pull request is a start at resolving #118 -- as mentioned in that issue, I've created a function _get_child_memory that iterates through all child processes yielding their memory. This is summed if the include_children flag is true.

I've also updated the reference from #71 to wrap the entire yield in try/except and yield 0.0 if the race condition occurs; but this needs to be checked.

Including this into the main mprof library is the next step. I've created a demo mpmprof which logs child and main process memory separately in th same .dat file, then plots them correctly. Here is an example:

multiprocessing_example

The next steps are to merge mpmprof with mprof with some flag, e.g. independent_children or something like that (suggestions welcome) to make it behave like mpmprof and to have the plot command correctly handle both cases.

raise NotImplementedError('The psutil module is required when to'
' monitor memory usage of children'
' processes')
raise NotImplementedError((
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about this, I think this was just a carry over from the historical merge; I can reset back to the original if needed.

mpmprof Outdated
lines += 1

# Collect memory usage of spawned children and write to profile
for idx, mem in enumerate(mp._get_child_memory(proc.pid)):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So here is the line where I independently collect the memory of each child and add them to the .dat file with the "CHLD#" key. If include_children=True then "main" will be the total memory consumed, and if not, it will be only the main process, but all processes will be plotted with independent_children=True or whatever flag we come up with.

mpmprof Outdated
mpoint = (0, 0)

# Plot all of the series, the main process and the child.
for proc, data in series.items():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And then obviously, plot needs to be adapted to plot all the children.

@bbengfort
Copy link
Contributor Author

mpmprof uses argparse e.g. #128

@fabianp
Copy link
Collaborator

fabianp commented Mar 18, 2017

Thanks @bbengfort! . Do you think it is possible to have only one executable file, i.e., merge mpmprof into mprof?

@bbengfort
Copy link
Contributor Author

@fabianp yes, it's possible; just not very straightforward. I'll take a crack at it.

@fabianp
Copy link
Collaborator

fabianp commented Mar 20, 2017 via email

@bbengfort
Copy link
Contributor Author

@fabianp ok, I've pushed a version that does it - would you mind taking a look and running your tests/examples?

@fabianp
Copy link
Collaborator

fabianp commented Mar 20, 2017

Great work @bbengfort ! The code looks great, let me try it on some of my own problems. In the meantime, you can add your name to the Authors part of the README :-)

@bbengfort
Copy link
Contributor Author

@fabianp thanks! I've updated the README with the example and usage and also added max value markers to the plot for the largest child.

@fabianp
Copy link
Collaborator

fabianp commented Mar 20, 2017

If I run the example with mprof run -M multiprocessing_example.py then it fails with an _pickle.PicklingError exception. It works however with mprof run -M python multiprocessing_example.py (not the python after -M). However, both should give the same result as the python mode should be picked up by the .py extension.

README.rst Outdated
the total memory of the program as well as each child individually.

.. warning:: currently the child tracking only works if a ``stream`` is provided to the ``profile`` (e.g. from the command line or in the decorator).

Copy link
Collaborator

Choose a reason for hiding this comment

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

How hard do you think it would be to remove this limitation?, i.e., to track children even if the function is not @Profile'd

README.rst Outdated
the total memory of the program as well as each child individually.

.. warning:: Currently the child tracking only works if a ``stream`` is provided to the ``profile`` (e.g. from the command line or in the decorator). If you are using the API to retrieve values then the flag will not do anything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am confused, as the example works fine even without decorated functions

Copy link
Contributor Author

@bbengfort bbengfort Mar 20, 2017

Choose a reason for hiding this comment

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

I meant that the stream argument has to be passed in, if stream = None then I can't return multiple values. I could if you wanted me to return a list instead of a single float. I didn't necessarily mean it wouldn't work with the decorator.

Perhaps I could rephrase as follows:

"Currently tracking individual children requires a reporting output stream; if you'd like direct access to the memory usage of individual children, see the _get_child_memory function."

Though of course, that then points the user to an internal function. I'm open to suggestions.

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've put placeholder comments in the code (line 363 and 403) where this issue occurs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, thanks for the explanation. I think the API would be simpler if we allow memory_usage to return a nested list instead embedding this into the stream. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've updated memory_usage to return a nested list and updated the README accordingly.

@bbengfort
Copy link
Contributor Author

Unfortunately, I think that the pickle error is a product of multiprocessing library: functions can only be pickled if they're defined at the top level and when you run it by creating a subprocess in mprof, the example function is no longer at the top level and therefore cannot be pickled.

I can see if I can change the example (though that may be difficult), but I also wanted to note that the problem exists even with the original version of the mprof, so it's not related to the code changes I proposed here. It's likely that any user that uses the multiprocessing library would have to use the python command to ensure their code is serialized/forked correctly.

http://stackoverflow.com/questions/8804830/python-multiprocessing-pickling-error

else:
ret.append(mem_usage)
if multiprocess:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the first place where I don't return multiple values and instead warn

else:
ret.append(mem_usage)

if multiprocess:
warnings.warn("use include_children not multiprocess without a stream")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the second place where I don't return multiple values and instead warn

@fabianp
Copy link
Collaborator

fabianp commented Mar 20, 2017

For the pickling, what I find strange is that both commands (with and without the "python" word in mprof) should be identical, as per lines 217-223 of mprof. Not sure why multiprocessing fails only on one of these cases since they should be executed the same way.

@bbengfort
Copy link
Contributor Author

So I see what you mean, however there is a subtle difference. The args to Popen if options.python = True are:

['python', '-m', 'memory_profiler', '--timestamp', '-o', 'mprofile_20170321080505.dat', 'examples/multiprocessing_example.py']

Whereas the args to Popen if passed as an executable are

`['python', 'examples/multiprocessing_example.py']` 

In the first case, __main__ is memory_profiler.__main__ and examples/multiprocessing_example.py is run with execfile() on L1108 or exec() on L1119. This is correctly implemented (as discussed here) by embedding the exec into its own namespace (ns), thus making the exec'd file the equivalent of an import -- as a result, the functions inside examples/multiprocessing_example.py are not at the top level (but are nested under ns) and therefore cannot be pickled.

So that took me a while to figure out, but I think it makes sense. What do you think?

@fabianp fabianp merged commit d333fda into pythonprofilers:master Mar 21, 2017
@fabianp
Copy link
Collaborator

fabianp commented Mar 21, 2017

Thanks for the explanation and your time. I've merged the PR and added a small patch (52c5788) so that the example can be run without the python keyword.

Thanks!

@bbengfort
Copy link
Contributor Author

Sure thing, let me know if there is anything else I can help with; sorry that this feature took so long!

except psutil.NoSuchProcess:
# https://github.com/fabianp/memory_profiler/issues/71
pass
mem += sum(_get_child_memory(process, meminfo_attr))

Choose a reason for hiding this comment

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

Hi, quick question, as processes in modern linux share common memory after forking from their parent and only copy it on writes, wouldn't this sum report the wrong number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I'm not sure of the behavior of psutil with regards to linux forks; I simply used the same meminfo_attr as the summation function. If that's true then this is indeed a problem, but I'd guess that we'd have to dig into psutil to figure it out.

@fabianp
Copy link
Collaborator

fabianp commented Jun 12, 2017 via email

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.

3 participants