ENH: add callback that shows memory and cpu usage. #1024
Conversation
Yeah, I don't want to add dependencies for such a small thing. Usually when we have an optional dependency that is essential for some part (like Since that is not really the case here, I think the way to go is to import lazily in |
odl/solvers/util/callback.py
Outdated
| def __call__(self, _): | ||
| """Print the memory and CPU usage""" | ||
| if self.iter % self.step == 0: | ||
| print('CPU usage (% each core): {}'.format( |
There was a problem hiding this comment.
I would prefer an optional format string as is the case in some other callbacks. Also keeps it too one line by default.
There was a problem hiding this comment.
Suggestion:
- remove swap (not really needed)
- use format strings for each resource separately, like
fmt_cpu,fmt_mem, with sensible defaults - add flags
print_cpuandprint_memto switch on/off
odl/solvers/util/callback.py
Outdated
| @@ -854,6 +856,52 @@ def __repr__(self): | |||
| self.logy) | |||
|
|
|||
|
|
|||
| class CallbackPrintMemory(SolverCallback): | |||
There was a problem hiding this comment.
It prints more than memory so perhaps some other name. CallbackPrintHardwareUse or something.
There was a problem hiding this comment.
True, Usage it would be in any case.
odl/solvers/util/callback.py
Outdated
| @@ -13,6 +13,7 @@ | |||
| from future import standard_library | |||
| standard_library.install_aliases() | |||
|
|
|||
| import psutil | |||
There was a problem hiding this comment.
As discussed, Lazily import inside the call.
|
I am working on the updates. Where in the doc should the dependency be mentioned? I tried to check how it was done for ray transform (with astra) and FFT (with pyfftw), but I could not see a clear pattern... |
Also updates in doc, parameters in init, and other things.
|
I think that all comments are addressed. However, as my previous comment indicates I was not sure of where in the doc to write about the requirements on |
odl/solvers/util/callback.py
Outdated
| ---------- | ||
| step : positive int, optional | ||
| Number of iterations between output. Default: 1 | ||
| print_cpu : bool, optional |
There was a problem hiding this comment.
Suggestion: scrap the "print_xyz" things and infer it from the fmt strings (perhaps with a rename), e.g.
swap, default is string but anything that evaluates to False (e.g. do a if swap:) means don't print.
Much more simple interface.
There was a problem hiding this comment.
But you can keep the old names for the format string parameters, they're good. Add a sentence in the format string docs then that "An empty format string disables printing of XYZ usage".
odl/solvers/util/callback.py
Outdated
| Raises | ||
| ------ | ||
| ImportError | ||
| If the package ``psutil`` is not installed. |
There was a problem hiding this comment.
This is enough imo, no need to spam doc with info in this dependency. In 98% of cases people will get the error and run pip install psutil without even reading the doc.
There was a problem hiding this comment.
I'd write a sentence
This callback requires the ``psutil`` package.
in the class doc and just let the error propagate. No Raises section (that is intended for exceptions that we raise, not the system or a third-party package).
odl/solvers/util/callback.py
Outdated
| def __call__(self, _): | ||
| """Print the memory and CPU usage""" | ||
|
|
||
| try: |
There was a problem hiding this comment.
I prefer propagating the error as is, I.e. no try catch
odl/solvers/util/callback.py
Outdated
| def __repr__(self): | ||
| """Return ``repr(self)``.""" | ||
| optargs = [('step', self.step, 1)] | ||
| inner_str = signature_string([], optargs) |
Change inputs to init method, and a number of other minor things.
|
I hope this should do :-) |
|
Related to this, in the debugging in #1014 we also use |
That kind of functionality definitely useful, but I'm a bit skeptical as to whether that requires a separate callback. The |
Add callback that prints memory and cpu usage. As in the code example in issue #1014