vine: fixed_location task groups #3787
Conversation
|
This currently works (from some testing i have done). It does some pretty expensive searches and is a bit invasive to the code. I wanted to open it up at this stage in case anyone has thoughts on the idea and implementation. |
colinthomas-z80
force-pushed
the
task_groups
branch
from
f4de28d to
a09c990
Compare
colinthomas-z80
force-pushed
the
task_groups
branch
from
5ab8af6 to
35d3e30
Compare
|
Colin, this has been sitting here for a while but based on our recent discussions I am getting more comfortable with the approach. Do you think this is ready to merge? Does it need more testing? Rebasing? |
colinthomas-z80
force-pushed
the
task_groups
branch
from
65051f5 to
aed1957
Compare
|
Accidentally closed? |
|
Yes I’m not sure why but I’ll fix it up so it doesn’t have all the outside commits. Regarding merging in it should not create any issues with the feature turned off by default. While using the task groups there are still a few holes in the policy for error handling. |
|
Per our discussion today, waiting until this works smoothly with the Montage+Parsl example, then will be more confident in merging... |
colinthomas-z80
force-pushed
the
task_groups
branch
from
0e17e92 to
2119c31
Compare
colinthomas-z80
force-pushed
the
task_groups
branch
from
2119c31 to
e62d595
Compare
taskvine/src/worker/vine_sandbox.c
Outdated
| break; | ||
| case VINE_CACHE_STATUS_UNKNOWN: | ||
| case VINE_CACHE_STATUS_FAILED: | ||
| if (p->task->group_id) { |
There was a problem hiding this comment.
Re our discussion today, move this failure-handling logic to task_can_run_eventually.
If a task has an input file that is UNKNOWN, check it see if another known tasks produces it as an output file.
If so, then the task can stay until it becomes runnable.
|
@colinthomas-z80 Please see conflicts above... |
colinthomas-z80
force-pushed
the
task_groups
branch
from
f79dab9 to
78cba11
Compare
colinthomas-z80
force-pushed
the
task_groups
branch
from
78cba11 to
2fc3180
Compare
|
@colinthomas-z80 this is looking pretty close, except for the task_can_run_eventually bit. What do you think -- is there something else missing here before we are ready to merge? |
|
Yes that part could be a bit more elegant. Before merging I want to clean up the worker debug logging and I am testing a change that fixes an issue with recovery tasks. |
|
ok, thanks for the update |
colinthomas-z80
force-pushed
the
task_groups
branch
from
4c03f1b to
587eb35
Compare
|
RTM |
taskvine/src/manager/vine_manager.c
Outdated
| return 0; | ||
| } | ||
|
|
||
| if (task->group_id && (task->refcount > 1)) { |
There was a problem hiding this comment.
Does it need task->group_id = NULL? inside this if?
There was a problem hiding this comment.
I don't think so since the task is getting deleted afterwards
There was a problem hiding this comment.
I'd prefer to have the if (task->group_id) { only, set it to NULL inside the if, and do not check for the refcount. The only place where we should be checking for the refcount is when deleting the task, otherwise it gets tricky if we ever call functions in a different context.
| char *id = string_format("%d", group_id_counter++); | ||
| struct list *l = list_create(); | ||
|
|
||
| t->group_id = id; |
There was a problem hiding this comment.
why is group_id a string? Let's keep an int if possible so there is less chance of memory leaks/segfaults.
There was a problem hiding this comment.
it is because the group_id is a key to a hash table. They used to be a full uuid string but that was deemed to be excessive since the number of groups does not get very large.
There was a problem hiding this comment.
I suggest using struct itable then. It is like struct hash_table but the keys are ints.
3 participants
Proposed changes
With task groups enabled, detect groups of sequential, temp-file dependent tasks, and send them in batches to a single worker, avoiding the overhead and complexity of maintaining fixed_location policy.
Post-change actions
Put an 'x' in the boxes that describe post-change actions that you have done.
The more 'x' ticked, the faster your changes are accepted by maintainers.
make testRun local tests prior to pushing.make formatFormat source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)make lintRun lint on source code prior to pushing.Additional comments
This section is dedicated to changes that are ambitious or complex and require substantial discussions. Feel free to start the ball rolling.