Suggest making the arguments to this function kwarg-only so it's more readable (it would also allow for default values)

And maybe "output_fn" -> "done_cb" or "on_done". Otherwise I'd expect that "output_fn" specifies where to put results

This sounds like a callback instead of an "output_fn", is there a name involving "callback" that would fit?

Follow-up question, what is the signature of this function?

I'm not really sure what a reader is supposed to take away from this statement. What is an "iteration prediction function"? How is it used with next_step? Why is that relevant?

The code here is very challenging to read and looks almost like a state machine. I guess I was expecting something simpler (like taking the modulus of the current iteration or epoch). Is this additional complexity really necessary?

it is indeed exactly a finite state machine, yes
and we do need to track the states and transitions between them, so a finite state machine seems reasonable

Can you elaborate on why that's necessary (vs taking a modulus)?

I can e.g. give an example showing that it's not so simple.
So you take the modulus and you know your current state, is it enough?
You still need to know what was the previous state and what where in the sequence you are.
If the current state is active, and the previous is warmup - action is start_trace
If the current state is active, and the previous is inactive - action is start_warmup, followed by start_trace
If the current state is active, and the previous state is active, does it mean we don't need to do anything?
It depends, consider (wait=0, warmup=0, active=2) - sometimes we don't do anything, sometimes we do stop, save, warmup, start.

So you still have to consider a lot of possibilities and edge cases, then in that case, why not just encode states and actions explicitly in the map? The original version of the code that didn't use it was way less readable imo.

What actually happens when wait=0, warmup=0 and active=2? I would expect the following pattern:

ACTIVE ACTIVE ACTIVE ACTIVE ACTIVE...

Is this not correct?

"wait=0, warmup=0 and active=2" means don't wait, don't do warm up, trace for two iterations

then the sequence repeats, and we provide access to the traces once they are available to print/save

Oh interesting. So it's more correctly:

ACTIVE ACTIVE REPORT ACTIVE ACTIVE REPORT

?

"REPORT" is implied at the every end of enable_pred cycle, will make sure to make it clear from the docs

can you describe what enable_pred is, and also give an example below of using profiler with enable_pred?

an example of initialization will be useful here

1. The documentation does not explain what is next_step and does not explain what the user should do with it (the docs should say that users should call next_step once per training loop iteration, and that an event corresponding to the iteration will be recorded in this call)
2. There should be a link to record_function documentation here, otherwise record_function api is not clear

Give a more detailed error message, with an example of how to instantiate and pass enable_pred

Maybe this 'self.next_step()' should put before return? Or else the first step info will loss when self.enable_pre==None.

I think "enable predicate". It's probably not the best name, maybe "IterationSchedule" or something like that? (I'm bad at names too)

we're minimum at 3.6 now: https://github.com/pytorch/pytorch/blob/master/cmake/Dependencies.cmake#L963 so you're safe. Please update

And maybe "output_fn" -> "done_cb" or "on_done". Otherwise I'd expect that "output_fn" specifies where to put results

should you just have a default enable_pred which does this behavior?

in this case it's an endless tracing (for all iterations), then we should somehow represent it in EnablePred

prints are bad, use warnings module

I still feel it can be simplified. Can we replace the entire EnablePred with a function that returns one of the few enums of what to do on this iteration:

• NONE
• WARMUP (i.e. enable but not record)
• RECORD (i.e. enabled)
• RECORD_AND_FLUSH (i.e. enabled but dump a new iteration)

Then the profiler doesn't even need to depend on EnablePred class. It can be just an arbitrary callable with that maps step_num into action. Of course you'd provide the code you have here as the default implementation but the user can do their own. E.g. the simple one would be: enable_pred = lambda num: WARMUP if num < 100 else RECORD for simple profile that doesn't dump intermediate results.

This logic about state machine transition is best moved to Profiler directly

As for output_fn - what benefit does it have over just putting if p.step % 100 == 0: <my code> directly in the training loop? You can keep it, but I'd add it as a separate arg in addition to enable_pred so that both can be easily-passable lambdas

Good discussion here and above - I agree we should strive to simplify this as much as possible, and then make it as easy to understand as possible via good names and examples.

Warning: I haven't read the entire code yet so I might be missing the point - my comments are about user experience, not so much implementation details.

In my experience, a default behavior is be good for the majority of cases so I'll use that as example: one warmup iteration followed by three recorded iterations.
What frequently changes is when you want to trigger it (should allow simple modulo approach here I think) and what to do with the result. Less frequently, how many iterations to profile and which activities to include.

Given that, the standard use case ought to be something like:
trigger_when: p.step % 1000 == 100
handle_result: write_chrome_trace / print_summary / extract_metrics .... (list of predefined convenience functions)

i.e. trigger profiling at iteration 100, 1100 and so on. Important to note here is that profiling cannot be enabled while at the same time reporting. We need to warmup, record, then stop and process, in order for the overhead to be manageable during the record window.

To clarify the above, this will be the behavior:
1100: Warmup
1101-1102: Record
1103: Record, then stop and process, calling the handle_result function asynchronously when processing is complete. As much work as possible is deferred to the processing stage to minimize overhead during recording. Recording should not continue in parallel with processing for detailed profiling, but we also need to consider a light-weight continuous profiling use case, possibly streaming a trace or ongoing extraction of metrics.

@dzhulgakov suggestion sounds good, basically let's provide a way to define a sequence of states explicitly through lambda + suggested default impl

one caveat - we might then allow users to specify sequences like: warmup none warmup none, etc - so we'll need to check for that, but i guess that's fine - assuming most users use supplied default and the power users know what they are doing;

@gdankel I think both solutions above should basically allow these kind of use cases

Sure... As long as users don't have to deal with this level of detail in the majority of cases, it seems reasonable.

calling state again here leaks details. Can't profile object remember its current state instead?

why is keep_profiler true in this case?

so that we can also use it after the context manager is finished

I wonder whether we should still support use_cuda for backward compatibility?

Do we really want to allow people to pass callable here, instead of wait, warmup, active ints? Seems like writing a valid schedule would be hard and no one would do it?

will point to the user to use schedule()

trace is ready is not defined. "when trace has been collected for the specified number of steps"?

you shouldn't use assert for user-induced errors. Change it for raise and better error message

same here