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

Update Iterative Tasking Infrastructure #917

Merged
merged 5 commits into from
Aug 11, 2023
Merged

Conversation

pdmullen
Copy link
Collaborator

@pdmullen pdmullen commented Aug 9, 2023

PR Summary

I am submitting this PR on behalf of @jdolence who made these changes in lroberts36/merge-sparse-with-jdolence-sparse some time ago. In practice, this PR slightly modifies the task list infrastructure to update iterative tasking, in particular with regional dependencies. This PR fixes various failure modes exhibited in downstream codes whereby (1) iterative tasks failed to iterate, (2) hangs were experienced, or (3) unexpected results were obtained with MPI both with and without regional dependencies.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@pdmullen pdmullen changed the title WIP: Update Iterative Tasking Infrastructure Update Iterative Tasking Infrastructure Aug 9, 2023
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Thanks for porting this in. Sorry I missed it on the first batch of updates.

Copy link
Collaborator

@lroberts36 lroberts36 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for porting this. I did not go through the logic in detail, but that is probably not as useful as the test that it actually works in Riot.

Comment on lines +55 to +59
template <class T, class U, class... Args1, class... Args2>
TaskID AddTask(TaskID const &dep, TaskStatus (T::*func)(Args1...), U *obj,
Args2 &&...args) {
return this->AddTask_(TaskType::iterative, 1, dep, [=]() mutable -> TaskStatus {
return (obj->*func)(std::forward<Args>(args)...);
return (obj->*func)(std::forward<Args2>(args)...);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't immediately see why we need two separate variadic template parameters here. I am mostly just curious what issue the single list was causing. Maybe it was something to do with default arguments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately I can't remember exactly what situation led to this change, but I can give you the basic idea. What happened was that I wrote a task with some set of reasonable arguments (I thought) but the compiler yelled at me about type mismatches when I actually tried to add it to a task list. Don't hold me to this, but I think it was a const vs non-const issue. I convinced myself at the time that there was no reason we should be preventing people from writing tasks the way I had written the problematic one, and this transformation generalized the interface to allow it.

src/tasks/task_list.hpp Outdated Show resolved Hide resolved
@pdmullen pdmullen enabled auto-merge (squash) August 11, 2023 03:41
@pdmullen
Copy link
Collaborator Author

pdmullen commented Aug 11, 2023

Extended regression is failing upon enabling auto-merge. Is this definitely pointing to an issue with the changes in this PR? Or could it be unrelated?

Asking before I jump down the rabbit hole.

@pdmullen pdmullen disabled auto-merge August 11, 2023 15:00
@pdmullen pdmullen enabled auto-merge (squash) August 11, 2023 15:14
@pdmullen pdmullen merged commit ab9d167 into develop Aug 11, 2023
@Yurlungur
Copy link
Collaborator

Extended regression is failing upon enabling auto-merge. Is this definitely pointing to an issue with the changes in this PR? Or could it be unrelated?

Given that automerge eventually let it go, sounds like the issue was just the test machine overloaded.

@Yurlungur Yurlungur deleted the pdmullen/iter-tasks-fix branch August 11, 2023 16:47
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.

4 participants