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 step functions #886

Merged
merged 7 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.

Still open on naming for the two new steps for the function. For now we have:

  • build_image
  • save_function

This does not include the API part for the frontend, which woudl be part of another task (FL-1518)

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 guilhem-barthes marked this pull request as ready for review April 11, 2024 09:31
@guilhem-barthes guilhem-barthes requested a review from a team as a code owner April 11, 2024 09:31
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 👍
Unsure about how the mocks are (re)defined in this PR, I feel like it's contrary to best practices.
Also need a rebase ^^

backend/builder/tests/test_task_build_image.py Outdated Show resolved Hide resolved
backend/orchestrator/resources.py Show resolved Hide resolved
backend/substrapp/tests/compute_tasks/test_outputs.py Outdated Show resolved Hide resolved
backend/substrapp/tests/tasks/test_compute_task.py Outdated Show resolved Hide resolved
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 🙏

@guilhem-barthes guilhem-barthes force-pushed the feat/add-profiling-step-functions branch from 49bb096 to 7b935bd Compare June 14, 2024 10:09
Signed-off-by: Guilhem Barthés <guilhem.barthes@owkin.com>
…o `orchestrator`

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 guilhem-barthes force-pushed the feat/add-profiling-step-functions branch from 159819f to 6beea40 Compare June 14, 2024 10:17
@guilhem-barthes guilhem-barthes enabled auto-merge (squash) June 14, 2024 10:17
@guilhem-barthes guilhem-barthes merged commit 854114f into main Jun 14, 2024
8 checks passed
@guilhem-barthes guilhem-barthes deleted the feat/add-profiling-step-functions branch June 14, 2024 10:23
guilhem-barthes added a commit to Substra/orchestrator that referenced this pull request Jun 14, 2024
## Description

<!-- Please reference issue if any. -->

<!-- Please include a summary of your changes. -->

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

- Substra/substra-backend#886

## How has this been tested?

<!-- Please describe the tests that you ran to verify your changes.  -->

## Checklist

- [ ] [changelog](../CHANGELOG.md) was updated with notable changes
- [ ] documentation was updated

---------

Signed-off-by: Guilhem Barthés <guilhem.barthes@owkin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants