parthenon-hpc-lab / parthenon

Parthenon AMR infrastructure
https://parthenon-hpc-lab.github.io/parthenon/
Other
105 stars 33 forks source link

Add capability to output task graph #1099

Closed lroberts36 closed 2 weeks ago

lroberts36 commented 1 month ago

PR Summary

This PR adds the functionality to output the graph of a TaskList, TaskRegion, or TaskCollection in the Graphviz format. I do not explicitly add the connectivity between TaskRegions, so they show up as disconnected graphs. It is pretty trivial to pull out the graph because of #917. Initially, I used the nameof library for writing out function names, but I am not actually convinced this is necessary. I think we get everything we want with

#define TF(...) std::string(#__VA_ARGS__), __VA_ARGS__

Let me know if you have thoughts on this. After reading the internet for a while, I am pretty sure that there is no way to get the names of tasks within task lists themselves since they are effectively function pointers and no name information is available (hence the need to allow passing a string naming the task).

Results look like (from the sparse advection test without refinement and a single block): Screen Shot 2024-06-07 at 1 51 18 PM

PR Checklist

lroberts36 commented 2 weeks ago

LGTM. Looks like now the user has to call WriteTaskGraph by hand? That's probably for the best.

Yeah, currently the user has to do something like std::cout << tc; to write the graph to screen. Patrick suggested having an option in the driver that would write the task graph to file on the first step.

If I interpret this correctly, the task graph appending is optional--and this doesn't break downstream?

Yes, this should have no impact on downstream codes. Although there is a label attached to all tasks now, it has no impact unless you call the output stream operator on a TaskCollection, TaskRegion, or TaskList. If you do call the output stream operator before completely building the task list, it is possible that it could break things since the output operator calls the graph completion task (which is required so that all the edges are correctly set before printing).

The vector of tasks that gets appended to for output of the graph is a temporary and has no impact on the internal task lists of Task*. Aside from the issue of calling the output operator to early and finalizing the graph to early, you should be able to write the task graph to file and then execute the task graph with no side effects.

Also could we support

PARTHENON_ADDTASK(tasklist, dependency, _VA_ARGS_)

as sugar for the call around TF?

I don't quite see what this would buy us?

Yurlungur commented 2 weeks ago

Yeah, currently the user has to do something like std::cout << tc; to write the graph to screen. Patrick suggested having an option in the driver that would write the task graph to file on the first step.

:+1: would be kind of nice to have the driver do it automatically.

Yes, this should have no impact on downstream codes. Although there is a label attached to all tasks now, it has no impact unless you call the output stream operator on a TaskCollection, TaskRegion, or TaskList. If you do call the output stream operator before completely building the task list, it is possible that it could break things since the output operator calls the graph completion task (which is required so that all the edges are correctly set before printing).

Not super important... but can we add guard rails for that?

The vector of tasks that gets appended to for output of the graph is a temporary and has no impact on the internal task lists of Task*. Aside from the issue of calling the output operator to early and finalizing the graph to early, you should be able to write the task graph to file and then execute the task graph with no side effects.

:+1:

I don't quite see what this would buy us?

I just find TF a little ugly and I like that macro better. But it's a taste thing. No need to add it.

lroberts36 commented 2 weeks ago

Yeah, currently the user has to do something like std::cout << tc; to write the graph to screen. Patrick suggested having an option in the driver that would write the task graph to file on the first step.

👍 would be kind of nice to have the driver do it automatically.

Yeah, agreed, but let's leave for a future PR.

Yes, this should have no impact on downstream codes. Although there is a label attached to all tasks now, it has no impact unless you call the output stream operator on a TaskCollection, TaskRegion, or TaskList. If you do call the output stream operator before completely building the task list, it is possible that it could break things since the output operator calls the graph completion task (which is required so that all the edges are correctly set before printing).

Not super important... but can we add guard rails for that?

I just included a check in AddTask that the task region containing it hasn't had BuildGraph called.

I just find TF a little ugly and I like that macro better. But it's a taste thing. No need to add it.

Josh and I thought this sort of pattern was a quick to implement, minimally invasive way to update old task list building code, but I agree that it can be a little obscure (which partially inspired the task function acronym choice...).

Yurlungur commented 2 weeks ago

Yeah, agreed, but let's leave for a future PR.

:+1:

I just included a check in AddTask that the task region containing it hasn't had BuildGraph called.

:+1:

Josh and I thought this sort of pattern was a quick to implement, minimally invasive way to update old task list building code, but I agree that it can be a little obscure (which partially inspired the task function acronym choice...).

Yeah. That's fine. Like I said it's a taste thing.