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

feat: add profiling steps #407

Merged
merged 8 commits into from
Jun 14, 2024
Merged

feat: add profiling steps #407

merged 8 commits into from
Jun 14, 2024

Conversation

guilhem-barthes
Copy link
Contributor

@guilhem-barthes guilhem-barthes commented Apr 11, 2024

Description

Allow transferring function profiling steps through the orchestrator, allowing the builder to register event without having to have http(s) requests to the backend api server (involving having access to the db to get credentials). This also allows having duration for the function available on every backend.

The drawback of this approach is, how it is relying on the event system already in place, the function profiling are also saved in the DB of the orchestrator.

Fixes FL-1519

Companion PR

How has this been tested?

Checklist

  • changelog was updated with notable changes
  • documentation was updated

Copy link

linear bot commented Apr 11, 2024

@guilhem-barthes
Copy link
Contributor Author

/e2e --tests substra-backend=feat/add-profiling-step-functions

@Owlfred
Copy link
Contributor

Owlfred commented Apr 11, 2024

End to end tests: ❌ FAILURE

Jobs status:

“You shall not pass!” ― Gandalf, The Lord of the Rings, The Fellowship of the Ring

@guilhem-barthes guilhem-barthes marked this pull request as ready for review April 11, 2024 09:07
@guilhem-barthes guilhem-barthes requested a review from a team as a code owner April 11, 2024 09:07
Copy link
Contributor

@SdgJlbl SdgJlbl 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 your work. A test in the SDK test suite is failing though 🤔

@@ -0,0 +1,3 @@
A new service called `ProfilingService` in charge of propagating profiling
- For now, only register and propagates profiling steps
- Introduce a new asset type, ASSET_PROFILING_STEP`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Introduce a new asset type, ASSET_PROFILING_STEP`
- Introduce a new asset type, `ASSET_PROFILING_STEP`

message ProfilingStep {
uint32 duration = 1;
string asset_key = 2;
string step = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the last two items are Enum, but that there is no way of encoding that in protobuf?

@@ -0,0 +1,2 @@
INSERT INTO asset_kinds(kind) VALUES ('ASSET_PROFILING_STEP')
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to add a table to store the Enum with the profiling steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the pattern we used for previous enums, and we didn't implemented the management of postgres enums in the code.

@SdgJlbl
Copy link
Contributor

SdgJlbl commented Apr 22, 2024

/e2e --tests substra-backend=feat/add-profiling-step-functions

@Owlfred
Copy link
Contributor

Owlfred commented Apr 22, 2024

End to end tests: ❌ FAILURE

Jobs status:

Darn it.

@guilhem-barthes
Copy link
Contributor Author

/e2e --tests substra-backend=feat/add-profiling-step-functions

@Owlfred
Copy link
Contributor

Owlfred commented May 21, 2024

End to end tests: ❌ FAILURE

Jobs status:

“Success is not final; failure is not fatal: It is the courage to continue that counts.” ―- Winston S. Churchill

@guilhem-barthes
Copy link
Contributor Author

/e2e --tests substra-backend=feat/add-profiling-step-functions

@Owlfred
Copy link
Contributor

Owlfred commented May 27, 2024

End to end tests: ❌ FAILURE

Jobs status:

“Just Keep Swimming!” ― Dory, Finding Nemo

Signed-off-by: Guilhem Barthés <guilhem.barthes@owkin.com>
Signed-off-by: Guilhem Barthés <guilhem.barthes@owkin.com>
Signed-off-by: Guilhem Barthés <guilhem.barthes@owkin.com>
Signed-off-by: Guilhem Barthés <guilhem.barthes@owkin.com>
Signed-off-by: Guilhem Barthés <guilhem.barthes@owkin.com>
Signed-off-by: Guilhem Barthés <guilhem.barthes@owkin.com>
Signed-off-by: Guilhem Barthés <guilhem.barthes@owkin.com>
@guilhem-barthes
Copy link
Contributor Author

/e2e --tests substra-backend=feat/add-profiling-step-functions

@Owlfred
Copy link
Contributor

Owlfred commented May 27, 2024

End to end tests: ✔️ SUCCESS

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

@guilhem-barthes
Copy link
Contributor Author

/e2e --tests sdk,substrafl,frontend,documentation,mnist substra-backend=feat/add-profiling-step-functions

@Owlfred
Copy link
Contributor

Owlfred commented May 27, 2024

End to end tests: ✔️ SUCCESS

You rock!

Signed-off-by: Guilhem Barthés <guilhem.barthes@owkin.com>
@guilhem-barthes guilhem-barthes merged commit db00c49 into main Jun 14, 2024
7 checks passed
@guilhem-barthes guilhem-barthes deleted the feat/add-profiling-steps branch June 14, 2024 10:23
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.

3 participants