feat: update computeplan status #827
Conversation
guilhem-barthes
changed the title
guilhem-barthes
force-pushed
the
feat/update-computeplan-status
branch
2 times, most recently
from
c3cb2f1
to
6b8b76a
Compare
backend/api/models/computeplan.py
Outdated
| compute_plan_status = self.Status.PLAN_STATUS_CREATED | ||
| elif stats["waiting_builder_slot_count"] == stats["task_count"]: | ||
| compute_plan_status = self.Status.PLAN_STATUS_CREATED |
There was a problem hiding this comment.
You could group the two in the same if.
Also, the self.Status.PLAN_STATUS_TODO was completely removed. Is that on purpose ? If yes, why ?
There was a problem hiding this comment.
Yeah I will merge it if you think it is cleaner!
So for the status changes:
EMPTYandWAITINGhave been merged intoCREATEDwhich states only the existence of CP (EMPTY being just CREATED with 0 tasks)WAITINGreflected having one task inWAITING. This became in our new task modelWAITING_FOR_PARENTS, which happen after the build (which would be classified has doing, because it is actually working on a task). With product, we didn't think that the compute plan status needed the granularity of compute tasks (i.e. needed 3 types of waiting, which could be reached by a CP at the same time).TODOwas a reflection of a compute task being ready to be ran (built + execution). For compute task statuses, this status becameWAITING_FOR_EXECUTOR_SLOT. This is not the first action done on the task and a compute plan could also have tasks in different waiting states at the same time (therefore what todo would mean? One task has to be built? executed? etc). This also refers to the previous point about waiting.
There was a problem hiding this comment.
ok, so you mean that the case stats["doing_count"] == 0 and stats["done_count"] == 0: is in fact that a task is WAITING_FOR_EXECUTOR_SLOT, so the CP should still be in DOING ?
There was a problem hiding this comment.
Yes, because BUILDING is part of doing. That's why we renamed DOING in EXECUTING, because DOING encompasses BUILDING and EXECUTING. I don't think we could reach a state where we have neither BUILDING or EXECUTING with a compute plan in DOING
You are raising an interesting point, I think I should revert renaming doing_count to executing_count and adding BUILDING as part of it
There was a problem hiding this comment.
After second thoughts, it seems like we should keep executing_count as these metrics refers to task statuses (explicitely providing a count for each status) rather than a compute task one
guilhem-barthes
requested review from
ThibaultFy
and removed request for
ThibaultFy
guilhem-barthes
force-pushed
the
feat/update-computeplan-status
branch
from
8af6d79
to
de3d33b
Compare
Signed-off-by: Guilhem Barthés <[email protected]>
Signed-off-by: Guilhem Barthés <[email protected]>
Signed-off-by: Guilhem Barthés <[email protected]>
Signed-off-by: Guilhem Barthés <[email protected]>
Signed-off-by: Guilhem Barthés <[email protected]>
Signed-off-by: Guilhem Barthés <[email protected]>
Signed-off-by: Guilhem Barthés <[email protected]>
guilhem-barthes
force-pushed
the
feat/update-computeplan-status
branch
from
de3d33b
to
705fa3d
Compare
* chore: rename ComputeTask `DOING` to `EXECUTING` Signed-off-by: Guilhem Barthés <[email protected]> * feat: replace ComputePlan `EMPTY`, `TODO` & `WAITING`by `CREATED` Signed-off-by: Guilhem Barthés <[email protected]> * docs: changelog Signed-off-by: Guilhem Barthés <[email protected]> * chore: remove duplicate Signed-off-by: Guilhem Barthés <[email protected]> * chore: rename `create_created_cp` to `create_cp` Signed-off-by: Guilhem Barthés <[email protected]> * chore: refactor `update_status` Signed-off-by: Guilhem Barthés <[email protected]> * chore: separate migrations Signed-off-by: Guilhem Barthés <[email protected]> --------- Signed-off-by: Guilhem Barthés <[email protected]>
Description
Fixes FL-1422
How has this been tested?
CI Substra/substra-tests#320
Checklist