refactor: remove parent task key #112

guilhem-barthes · 2022-12-23T13:23:35Z

Companion PR

refactor: remove parent task key substra-backend#554

Description

Remove compute_task_parents:
in ComputeTask
in gRPC communication
remove expanded_compute_tasks

How has this been tested?

CI

Checklist

changelog was updated with notable changes
documentation was updated

guilhem-barthes · 2022-12-23T13:30:02Z

/e2e --refs substra-backend=feat/remove-parent_task_key

Owlfred · 2022-12-23T13:30:04Z

End to end tests: ❌ FAILURE

Jobs status:

Tests Benchmark: ⏭️
Tests Distributed: ❌
Tests Standalone: ✔️

Sorry, try again.

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

guilhem-barthes · 2023-01-02T13:59:44Z

/e2e --refs substra-backend=feat/remove-parent_task_key

Owlfred · 2023-01-02T13:59:46Z

End to end tests: ✔️ SUCCESS

“It’s alive! It’s alive!” ― Henry Frankenstein, Frankenstein

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

ThibaultFy · 2023-01-02T15:36:47Z

/e2e --refs substra-backend=feat/remove-parent_task_key --tests sdk,frontend --mode standalone

Owlfred · 2023-01-02T15:36:49Z

End to end tests: ✔️ SUCCESS

ThibaultFy

Thanks !!

ThibaultFy · 2023-01-02T16:19:54Z

CHANGELOG.md

+
+## Removed
+
+- field `parent_task_keys` in `ComputeTask`


Put the PR number

ThibaultFy · 2023-01-02T16:42:31Z

lib/service/computetaskstate.go

@@ -184,7 +184,7 @@ func (s *ComputeTaskService) propagateDone(triggeringParent, child *asset.Comput
 	}

 	// loop over parent, only change status if all parents are DONE
-	for _, parentKey := range child.ParentTaskKeys {
+	for _, parentKey := range GetParentTaskKeys(child.Inputs) {


Might worth testing if it take too long for big inputs (with 1000 datasamples for instance)

The e2e failes on SubstraFL and Benchmarks, seems to be because of a timeout error....

ThibaultFy · 2023-01-02T16:47:06Z

/e2e --refs substra-backend=feat/remove-parent_task_key --tests substrafl --mode standalone

Owlfred · 2023-01-02T16:47:08Z

End to end tests: ❌ FAILURE

Jobs status:

Tests Benchmark: ⏭️
Tests Distributed: ⏭️
Tests Standalone: ❌

It'll stay between us, no one needs to know.

ThibaultFy · 2023-01-02T16:47:29Z

/e2e --refs substra-backend=feat/remove-parent_task_key --tests NONE --mode standalone --benchmark mnist,camelyon

Owlfred · 2023-01-02T16:47:31Z

Something happened while processing your message: error: unknown option '--benchmark'
(Did you mean --benchmarks?)

ThibaultFy · 2023-01-02T16:48:01Z

/e2e --refs substra-backend=feat/remove-parent_task_key --tests NONE --mode standalone --benchmarks mnist,camelyon

Owlfred · 2023-01-02T16:48:03Z

End to end tests: ❌ FAILURE

Jobs status:

Tests Benchmark: ❌
Tests Distributed: ⏭️
Tests Standalone: ✔️

“Houston, we have a problem.” ― Jim Lovell, Apollo 13

ThibaultFy

E2E tests fail on SubstraFL and Benchmarks 😞

ThibaultFy · 2023-01-03T07:59:55Z

lib/service/computetaskstate.go

@@ -184,7 +184,7 @@ func (s *ComputeTaskService) propagateDone(triggeringParent, child *asset.Comput
 	}

 	// loop over parent, only change status if all parents are DONE
-	for _, parentKey := range child.ParentTaskKeys {
+	for _, parentKey := range GetParentTaskKeys(child.Inputs) {


The e2e failes on SubstraFL and Benchmarks, seems to be because of a timeout error....

ThibaultFy · 2023-01-03T08:20:11Z

End to end tests: ❌ FAILURE

Jobs status:

Tests Benchmark: ❌

Tests Distributed: ⏭️

Tests Standalone: ✔️

Epic fail.

For the Mnist benchmark, the performances are impacted:
Expected: [0.8118, 0.7792, 0.9042, 0.8814, 0.9176, 0.9348, 0.9434, 0.9456, 0.9564, 0.9556]
Actual perf: [0.8118, 0.7792, 0.8388, 0.788, 0.869, 0.797, 0.8556, 0.8084, 0.866, 0.819]

It would be interested to launch a CP locally and check how the graph of tasks is build.

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

guilhem-barthes · 2023-01-03T17:06:07Z

/e2e --refs substra-backend=feat/remove-parent_task_key --tests sdk,substrafl,frontend --benchmarks mnist,camelyon

Owlfred · 2023-01-03T17:06:09Z

End to end tests: ✔️ SUCCESS

“Carpe diem. Seize the day, boys.” ― John Keating, Dead Poets Society

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

ThibaultFy

Nice fix, can't wait to see it in action ;)

ThibaultFy · 2023-01-09T10:49:36Z

CHANGELOG.md

+## Removed
+
+- field `parent_task_keys` in `ComputeTask` ([#112](https://github.com/Substra/orchestrator/pull/112))
+- view `expanded_compute_tasks` ([#112](https://github.com/Substra/orchestrator/pull/112))


You can just put #112, github does the link to the PR automatically :)

ThibaultFy · 2023-01-09T10:51:23Z

lib/service/computetaskstate.go

@@ -204,7 +199,7 @@ func (s *ComputeTaskService) propagateDone(triggeringParent, child *asset.Comput
 			return nil
 		}
 	}
-	err := s.applyTaskAction(child, transitionTodo, fmt.Sprintf("Last parent task %s done", triggeringParent.Key))
+	err = s.applyTaskAction(child, transitionTodo, fmt.Sprintf("Last parent task %s done", triggeringParent.Key))


For my info: why remove the ":" here ?

guilhem-barthes mentioned this pull request Dec 23, 2022

refactor: remove parent task key Substra/substra-backend#554

Merged

2 tasks

guilhem-barthes added 3 commits December 23, 2022 14:50

refactor: remove parent_task_key from ComputeTask

b22c252

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

refactor: remove view expanded_compute_tasks

65eff8e

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

docs: add changelog

cb7bdce

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

guilhem-barthes force-pushed the feat/remove-parent_task_key branch from 88f5ac2 to cb7bdce Compare December 23, 2022 13:50

guilhem-barthes added 6 commits January 2, 2023 11:24

fix: Remove parent_task_keys from events

3ed665e

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

fix: Typo in migration 52

f69c4d3

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

fix: Typo in migration 52

4b90948

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

fix: drop condition in migration 52

35f8a05

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

fix: revert changes in chaincode/ledger/dbal_computetask.go

5c0e911

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

refactor: parent task index creation in ledger

ae3082c

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

fix: replace parent_task_keys by parentTaskKeys

daa912e

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

guilhem-barthes marked this pull request as ready for review January 2, 2023 14:39

guilhem-barthes requested review from ThibaultFy, oleobal and Milouu as code owners January 2, 2023 14:39

ThibaultFy approved these changes Jan 2, 2023

View reviewed changes

ThibaultFy requested changes Jan 3, 2023

View reviewed changes

feat: add a function to find children

e18a3b7

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

guilhem-barthes added 3 commits January 4, 2023 15:25

fix: fix typo

bcd7c6c

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

docs: update changelog

21577cf

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

fix(tests): add GetComputerTaskParents mock

a71b042

Signed-off-by: Guilhem Barthes <guilhem.barthes@owkin.com>

ThibaultFy approved these changes Jan 9, 2023

View reviewed changes

guilhem-barthes merged commit 5fc8d1c into main Jan 9, 2023

guilhem-barthes deleted the feat/remove-parent_task_key branch January 9, 2023 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove parent task key #112

refactor: remove parent task key #112

guilhem-barthes commented Dec 23, 2022 •

edited

guilhem-barthes commented Dec 23, 2022

Owlfred commented Dec 23, 2022 •

edited

guilhem-barthes commented Jan 2, 2023

Owlfred commented Jan 2, 2023 •

edited

ThibaultFy commented Jan 2, 2023

Owlfred commented Jan 2, 2023 •

edited

ThibaultFy left a comment

ThibaultFy Jan 2, 2023

ThibaultFy Jan 2, 2023

ThibaultFy Jan 3, 2023

ThibaultFy commented Jan 2, 2023

Owlfred commented Jan 2, 2023 •

edited

ThibaultFy commented Jan 2, 2023

Owlfred commented Jan 2, 2023

ThibaultFy commented Jan 2, 2023

Owlfred commented Jan 2, 2023 •

edited

ThibaultFy left a comment

ThibaultFy Jan 3, 2023

ThibaultFy commented Jan 3, 2023 •

edited

guilhem-barthes commented Jan 3, 2023

Owlfred commented Jan 3, 2023 •

edited

ThibaultFy left a comment

ThibaultFy Jan 9, 2023

ThibaultFy Jan 9, 2023

refactor: remove parent task key #112

refactor: remove parent task key #112

Conversation

guilhem-barthes commented Dec 23, 2022 • edited

Companion PR

Description

How has this been tested?

Checklist

guilhem-barthes commented Dec 23, 2022

Owlfred commented Dec 23, 2022 • edited

guilhem-barthes commented Jan 2, 2023

Owlfred commented Jan 2, 2023 • edited

ThibaultFy commented Jan 2, 2023

Owlfred commented Jan 2, 2023 • edited

ThibaultFy left a comment

Choose a reason for hiding this comment

ThibaultFy Jan 2, 2023

Choose a reason for hiding this comment

ThibaultFy Jan 2, 2023

Choose a reason for hiding this comment

ThibaultFy Jan 3, 2023

Choose a reason for hiding this comment

ThibaultFy commented Jan 2, 2023

Owlfred commented Jan 2, 2023 • edited

ThibaultFy commented Jan 2, 2023

Owlfred commented Jan 2, 2023

ThibaultFy commented Jan 2, 2023

Owlfred commented Jan 2, 2023 • edited

ThibaultFy left a comment

Choose a reason for hiding this comment

ThibaultFy Jan 3, 2023

Choose a reason for hiding this comment

ThibaultFy commented Jan 3, 2023 • edited

guilhem-barthes commented Jan 3, 2023

Owlfred commented Jan 3, 2023 • edited

ThibaultFy left a comment

Choose a reason for hiding this comment

ThibaultFy Jan 9, 2023

Choose a reason for hiding this comment

ThibaultFy Jan 9, 2023

Choose a reason for hiding this comment

guilhem-barthes commented Dec 23, 2022 •

edited

Owlfred commented Dec 23, 2022 •

edited

Owlfred commented Jan 2, 2023 •

edited

Owlfred commented Jan 2, 2023 •

edited

Owlfred commented Jan 2, 2023 •

edited

Owlfred commented Jan 2, 2023 •

edited

ThibaultFy commented Jan 3, 2023 •

edited

Owlfred commented Jan 3, 2023 •

edited