Implemented requirements defined in DM-32675 #84
Conversation
SergeyPod
force-pushed
the
tickets/DM-32675
branch
2 times, most recently
from
1175468
to
9caab2d
Compare
There was a problem hiding this comment.
A few things about the changes and then a couple higher level things:
- Need a file in doc/changes
- To understand new chunk function I had to look at code not in this PR. The docstring for parameter dependencies in split_dependencies_by_tasks doesn't match code.
|
|
||
| Parameters | ||
| ---------- | ||
| tasks_dependency_map : `dict` of dependencies dictionary |
There was a problem hiding this comment.
Style: only type goes here, description goes on next line. Also fix the Returns just below here.
There was a problem hiding this comment.
Somewhere either here or where dependencies first used, it would be helpful to have a brief reminder what it is. For example, I didn't understand why len(dependencies) gave you the number of jobs in the task.
| chunk_id = hash(pseudo_input) % n_chunks | ||
| tasks_dependency_map_chunked.setdefault(task_name + "_chunk_" | ||
| + str(chunk_id), | ||
| {})[pseudo_input] \ |
There was a problem hiding this comment.
Since using task_name + "chunk" + str(chunk_id) in multiple places, please make it a variable or a function so if we need to change it we can safely do it in one place. Could use f-string. If zero-padding the chunk_id will make them show up in an order that would be helpful to someone monitoring the run, please consider doing that.
| tasks_chunked[task_name] = n_chunks | ||
| else: | ||
| tasks_dependency_map_chunked[task_name] = dependencies | ||
| tasks_dependency_map_chunked_updated_dep = {} |
There was a problem hiding this comment.
Need comment here briefly describing next block of code.
| self.tasks_steps[task_name + "_chunk_" + str(chunk_id)] = \ | ||
| self.tasks_steps[task_name] | ||
| self.tasks_cmd_lines[task_name + "_chunk_" + str(chunk_id)] = \ | ||
| self.tasks_cmd_lines[task_name] |
There was a problem hiding this comment.
No updates for self.tasks_inputs? Are those actually used anywhere? If not, they should be removed from the rest of the code.
There was a problem hiding this comment.
I've removed this data structure, was an artefact
SergeyPod
force-pushed
the
tickets/DM-32675
branch
from
9caab2d
to
371b9e3
Compare
Checklist
doc/changes