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: wait for compute plan & tasks to finish before using output assets #369

Merged
merged 7 commits into from
Jul 28, 2023

Conversation

guilhem-barthes
Copy link
Contributor

Related issue

# followed by the number of the issue

Summary

Notes

Useful in FL-994

Please check if the PR fulfills these requirements

  • If necessary, the changelog has been updated
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • The commit message follows the conventional commit specification
  • For any breaking changes, companion PRs have been opened on the following repositories:

@guilhem-barthes guilhem-barthes force-pushed the fix/wait-end-for-downloading-output branch from 56a361f to ddd373a Compare June 30, 2023 13:51
@guilhem-barthes guilhem-barthes marked this pull request as ready for review June 30, 2023 14:51
@guilhem-barthes guilhem-barthes requested a review from a team as a code owner June 30, 2023 14:51
@ThibaultFy
Copy link
Member

Why is it a fix ? and why are you changing this behaviour ? It looks like a feature change to discuss with product 😶

substra/sdk/backends/local/backend.py Outdated Show resolved Hide resolved
substra/sdk/backends/local/dal.py Outdated Show resolved Hide resolved
substra/sdk/backends/remote/backend.py Outdated Show resolved Hide resolved
@guilhem-barthes guilhem-barthes force-pushed the fix/wait-end-for-downloading-output branch from d2ca31f to 684f108 Compare July 20, 2023 10:03
@guilhem-barthes
Copy link
Contributor Author

/e2e

@Owlfred
Copy link

Owlfred commented Jul 20, 2023

End to end tests: ✔️ SUCCESS

@guilhem-barthes guilhem-barthes changed the title fix: wait for compute plan & tasks to finish before using output assets feat: wait for compute plan & tasks to finish before using output assets Jul 20, 2023
Copy link
Member

@ThibaultFy ThibaultFy 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 the PR, just a few comment on the changelog

CHANGELOG.md Outdated
@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- `wait_task` and `wait_compute_plan` to block execution until execution is over ([#368](https://github.com/Substra/substra/pull/368))
- `wait_completion` param on `wait_task` and `wait_compute_plan` to block execution until execution is over ([#368](https://github.com/Substra/substra/pull/368))
Copy link
Member

Choose a reason for hiding this comment

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

The param is not on wait_task and wait_compute_plan is it ?

CHANGELOG.md Outdated
Comment on lines 23 to 27
### Fixed

- `list_task_output_assets` and `get_task_output_asset` wait that the compute task is over before getting assets ([#369](https://github.com/Substra/substra/pull/369))
- `get_performances` wait that compute plan is over before getting assets ([#369](https://github.com/Substra/substra/pull/369))

Copy link
Member

Choose a reason for hiding this comment

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

Duplicate of the fixed section. And is it really a fix ? Looks like a feature no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RomainGoussault What is your opinion?

Copy link
Member

@ThibaultFy ThibaultFy left a comment

Choose a reason for hiding this comment

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

thanks :)

Signed-off-by: Guilhem Barthes <[email protected]>
Signed-off-by: Guilhem Barthes <[email protected]>
Signed-off-by: Guilhem Barthes <[email protected]>
Signed-off-by: Guilhem Barthes <[email protected]>
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes guilhem-barthes force-pushed the fix/wait-end-for-downloading-output branch from 4b3ee3d to a03071d Compare July 28, 2023 09:29
@guilhem-barthes guilhem-barthes merged commit 088eeca into main Jul 28, 2023
@guilhem-barthes guilhem-barthes deleted the fix/wait-end-for-downloading-output branch July 28, 2023 09:34
@Milouu Milouu mentioned this pull request Sep 5, 2023
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.

4 participants