basic profiling support #349
Conversation
|
This still have some caveats. Getting memory information on windows is somehow cumbersome without additional external dependencies (such as psutils. Problem with psutils is that it needs developer version of python on linux (gcc and python header), complicating installation somehow. Also missing tool to process profiling info yet. |
|
Thanks for this. Re psutil; it's kind of the de-facto standard for doing these kinds of OS queries; how about we depend on it as an optional dependency? That is, if it's available, the profiling will be that more thorough, and without it you get the basics. That way, users on Windows can If there is a pure-Python equivalent, that would of course be best. But in my experience it just isn't possible. :( |
|
I've changed it to use optional psutil module. Pure python isn't there - only resource module but that is available only on linux and darwin systems. |
There was a problem hiding this comment.
Sorry for the delay in looking at this, I've made some notes in-line, and overall I think there's one thing missing, which is that profiling currently happens per-process, but are stored as one global object, being overwritten each time.
Instead, I think we should store them together with the context.data["results"] data. And actually, looking through the code, I found that we're already collecting the duration of each process in the results dict.
pyblish-base/pyblish/plugin.py
Lines 532 to 534 in f8a2449
This would be a good spot to continue adding profile data I think, so let's move the profiling context manager to there.
Let me know what you think.
| profiling_path = os.path.join(tempfile.gettempdir(), | ||
| "{}-pyblish_profile.json".format(context.id)) | ||
|
|
||
| append_to_json(profiling_data, profiling_path) |
There was a problem hiding this comment.
Ah no, I don't think this is a good idea.
Why not let the user decide what to do with the profiling information?
context = util.publish()
with open("somefile.json", "w") as f:
json.dump(f, context.data["profiling"])GUIs could get a hold of it via the context as well, inbetween each process if need be, but likely also as a post-process of sorts.
There was a problem hiding this comment.
So you think that it shouldn't save to json file implicitly and it's up to user to handle data in context.data.get('statistics') (lets call it that) afterward? Just checking if I understand it correctly.
There was a problem hiding this comment.
Yes, don't write files without the user's knowledge. In the vast majority of cases, the user won't care for those files anyway. Profiling is primarily useful for the plug-in author.
|
|
||
| # collected using psutil module, None if it is not installed | ||
| try: | ||
| profiling_data['initial_memory_sage'] = profiler.initial_mem |
There was a problem hiding this comment.
Typo on _sage. The naming convention should be mixedCase for dictionary keys as well (JSON convention).
| result = __explicit_process(plugin, context, instance, action) | ||
| else: | ||
| result = __implicit_process(plugin, context, instance, action) | ||
| if not context.get('profiling'): |
There was a problem hiding this comment.
Typo here, should be context.data.get(), as below.
Unsure about whether profiling is the best name here. Grammatically it's a verb, describing some sort of state. That it is currently "profiling". As it isn't meant as that, it should probably we profile, but even that could be confused with an action. To "profile" something.
I'm thinking context.data["statistics"]
There was a problem hiding this comment.
I agree, statistics is much better.
| @@ -58,6 +60,45 @@ | |||
| STRICT_DATATYPES = EARLY_ADOPTER or STRICT_DATATYPES | |||
|
|
|||
|
|
|||
| class ProfilingTimer(): | |||
There was a problem hiding this comment.
I think we should stick with a @contextmanager, like it's being used elsewhere in Pyblish.
import contextlib
@contextlib.contextmanager
def profiling():
stats = {}
stats["startTime"] = time.time()
yield stats
stats["endTime"] = time.time()
with profiling() as stats:
...Alternatively..
stats = {}
with profiling(stats):
...| context.data['profiling'][plugin] = profiling_data | ||
| profiling_data["snapshot"] = { | ||
| "timestamp": int(time.time()), | ||
| "context": json.dumps(context, default=lambda o: '<not serializable>') |
There was a problem hiding this comment.
Not sure about "snapshot".. If the goal is capturing more data, I think we should instead favour a mechanism with which to let the plug-in author provide his own data, as mentioned in #348 (comment)
There was a problem hiding this comment.
I was thinking about snapshot as "map" if you will, that with all those profiling data you could later on visualize whole pyblish process with all its timings and what was changed in context each step. But yes, adding custom data from every plugin should be implemented also.
|
|
||
| context.data['profiling'][plugin] = profiling_data | ||
| profiling_data["snapshot"] = { | ||
| "timestamp": int(time.time()), |
There was a problem hiding this comment.
If we keep startTime from stats we can remove this.
|
Hi @antirotor, revisiting this long-forgotten issue and would like to merge it. It's a good feature! There are just a few notes left before a first 1.0 of the feature, if you're up for it it would be good to see those though. |
Sure thing, sorry for this zombie-state. I had more urgent things to do but it is still on my mind. I'll try to take care of it asap. |
Goal
It would be nice to have mode, that would generate profiling data for pyblish process and plugins. Producing lot of timing and other data, that can be later used for visualizing and debugging whole session.
Motivation
My idea is to capture whole context after each plugin is done as snapshots along with it's timing, memory consumtion, etc. to json. Then to have simple application processing this json file, drawing all data to canvas, so we'll be able to vizualize, what is going on with all data during publishing. This could be useful when debugging behaviour of pipelines where lot of plugins are doing many things.
This implements #348