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!: remove distributed mode #341

Merged
merged 13 commits into from
Dec 21, 2023
Merged

Conversation

ThibaultFy
Copy link
Member

@ThibaultFy ThibaultFy commented Dec 11, 2023

Description

closes FL-1329

How has this been tested?

Checklist

  • changelog was updated with notable changes
  • documentation was updated

Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy ThibaultFy force-pushed the feat/decreciate-distributed-mode branch from 115dc73 to d370bf5 Compare December 11, 2023 17:25
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk

@Owlfred
Copy link
Contributor

Owlfred commented Dec 12, 2023

End to end tests: ✔️ SUCCESS

Copy link

linear bot commented Dec 12, 2023

Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk,mnist,camelyon,substrafl

@Owlfred
Copy link
Contributor

Owlfred commented Dec 12, 2023

End to end tests: ✔️ SUCCESS

Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy ThibaultFy marked this pull request as ready for review December 12, 2023 14:37
@ThibaultFy ThibaultFy requested a review from a team as a code owner December 12, 2023 14:37
Comment on lines 16 to 18
subject:
organizations:
- {{ .Values.fabric.organization | default "Owkin" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, but maybe we want to keep this only with the default value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmmmh I'm not sure what this value does. I saw fabric no I deleted it... More info on this @guilhem-barthes ?

docs/api.md Outdated
## Consuming the gRPC API

When running in development mode, the orchestrator exposes a gRPC endpoint on port 9000.
gRPC reflection is enabled, and protobuf definitions are in [lib/assets](../lib/assets) directory.

**distributed mode**: requests **MUST** have the following 3 headers set:
Requests **MUST** have the following header set:
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I understand from the documentation, this whole paragaph about headers can be dropped I think?

docs/architecture.md Outdated Show resolved Hide resolved
Comment on lines 158 to 159
The same level of consistency is provided in standalone mode by leveraging [postgresql's isolation level](https://www.postgresql.org/docs/current/transaction-iso.html#XACT-SERIALIZABLE).
By enforcing serializable transaction, we make sure we don't rely on inconsistent data.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to keep these bullet points from the concurrent request processing.

docs/asset-dev.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this file is added here? 🤔
Was is not generated from a previous PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes I didn't see it. Yes it surely because of that !

@@ -8,34 +8,30 @@ Unless specified, all settings are mandatory.

| Env Var | mode | type | usage |
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we just drop the mode column in this table ? 🤔

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

### Removed

- BREAKING: remove all code related to the `distributed` mode, and mentions in schemas and documentation ([#341](https://github.com/Substra/orchestrator/pull/341))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it breaking if we already removed the functionality? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Did we ? I was thinkin that as the code was still there, the feature was too (even if we already cut some access to it). But if we consider the feature was already deactivated, then I fully agree with you :)

Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy ThibaultFy force-pushed the feat/decreciate-distributed-mode branch from 9546535 to c6cf08f Compare December 18, 2023 10:15
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk,mnist,camelyon,substrafl

@Owlfred
Copy link
Contributor

Owlfred commented Dec 21, 2023

End to end tests: ❌ FAILURE

Jobs status:

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

Copy link
Contributor

@guilhem-barthes guilhem-barthes left a comment

Choose a reason for hiding this comment

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

After PP review

@ThibaultFy ThibaultFy merged commit bd70dfc into main Dec 21, 2023
9 checks passed
@ThibaultFy ThibaultFy deleted the feat/decreciate-distributed-mode branch December 21, 2023 15:51
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.

5 participants