fix: add summary task pydantic validator #350
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
SdgJlbl
force-pushed
the
fix/compute-task-performance-issue
branch
from
60e7a18 to
6ae1eb9
Compare
SdgJlbl
changed the title
SdgJlbl
force-pushed
the
fix/compute-task-performance-issue
branch
5 times, most recently
from
7409af4 to
c0456dc
Compare
…ew backend API Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
SdgJlbl
force-pushed
the
fix/compute-task-performance-issue
branch
from
c0456dc to
94f1c4d
Compare
SdgJlbl
changed the title
SdgJlbl
changed the title
There was a problem hiding this comment.
Thanks for this PR! I am not sure about the abridged term. As much I can appreciate its meaning, I don't know if it would be understand correctly by most people. I don't know if SummaryTask would be better, or renaming the previous task as InterfacedTask/TaskWithInterface TaskWithIO. Maybe other @Substra/code-owners can give their opinion?
tests/datastore.py
Outdated
Comment on lines
174
to
175
| "start_date": "2021-11-12T09:28:06.947765800Z", | ||
| "end_date": "2021-11-12T09:30:04.705947400Z", |
There was a problem hiding this comment.
Were these change needed?
There was a problem hiding this comment.
I thought that the unit tests were testing more than what they are actually 😅
I'll create an issue about that and revert those changes.
SdgJlbl
changed the title
|
Not perfectly happy with the naming either, maybe renaming the object with the previous as |
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
SdgJlbl
force-pushed
the
fix/compute-task-performance-issue
branch
from
6de1097 to
07230e5
Compare
SdgJlbl
changed the title
substra/sdk/backends/local/dal.py
Outdated
| @@ -147,9 +147,15 @@ def list( | |||
| """Joins the results of the [local db](substra.sdk.backends.local.db.list) and the | |||
| [remote db](substra.sdk.backends.rest_client.list) in hybrid mode. | |||
| """ | |||
| local_assets = self._db.list(type_=type_, filters=filters, order_by=order_by, ascending=ascending) | |||
| if type_ == schemas.Type.SummaryTask: | |||
| # for the local DB, we need to map the abridged task type to the regular task type | |||
There was a problem hiding this comment.
Suggested change
| # for the local DB, we need to map the abridged task type to the regular task type | |
| # for the local DB, we need to map the summary task type to the regular task type |
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
SdgJlbl
force-pushed
the
fix/compute-task-performance-issue
branch
from
51ccbd8 to
5225d92
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issue
Substra/substra-backend#613
Summary
Fix calls to the API (substra-backend), adding a SummaryTask Pydantic validator to handle the new return format for lists of compute tasks.
Notes
A lot of changes are just replacements
list()->[]. Can be reviewed commit by commit to avoid noise.Please check if the PR fulfills these requirements