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

Feature/SK-946 | Add functionality for user defined server-functions #666

Merged
merged 25 commits into from
Oct 31, 2024

Conversation

viktorvaladi
Copy link
Contributor

@viktorvaladi viktorvaladi commented Jul 26, 2024

Adds functionality for user-defined aggregation, client selection, and client config. Also creates a new "update_handler" where some code from the aggregator and roundhandler relating to updates are moved.

Adds a new option to the start_session() CLI command:
client.start_session(server_functions=ServerFunctions)

User-defined code will be run in the new hook container

One simple example that highlights the new functionalities. See examples/server-functions/server_functions.py

@github-actions github-actions bot added minor feature New feature or request and removed minor labels Jul 26, 2024
@github-actions github-actions bot added the minor label Jul 26, 2024
@Wrede Wrede added the HOLD label Aug 19, 2024
@ahellander ahellander added major and removed minor labels Aug 24, 2024
@github-actions github-actions bot added the minor label Sep 2, 2024
@@ -574,6 +574,7 @@ def start_session(
helper: str = "",
min_clients: int = 1,
requested_clients: int = 8,
function_provider_path: str = None,
Copy link
Member

Choose a reason for hiding this comment

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

can't we have the FunctionProvider class as argument instead?

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 would be better since then the abstract class would prevent wrong structure on the class already at the API level. But I'm not sure what will happened with imports when the class gets stringified.. I'll investigate it :)

@Wrede Wrede removed the major label Sep 12, 2024
@github-actions github-actions bot added the github label Oct 8, 2024
@viktorvaladi viktorvaladi changed the title Feature/SK-946 | Add functionality for user defined aggregation Feature/SK-946 | Add functionality for user defined server-functions Oct 8, 2024
@Wrede Wrede added the HOLD label Oct 11, 2024
@@ -940,6 +945,7 @@ def start_session(
helper="",
min_clients=1,
requested_clients=8,
function_provider=None,
Copy link
Member

Choose a reason for hiding this comment

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

missing arg in doc string

print(f"Epoch {e}/{epochs-1} | Batch: {b}/{n_batches-1} | Loss: {loss.item()}")

# Metadata needed for aggregation server side
metadata = {
Copy link
Member

Choose a reason for hiding this comment

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

the naming of metadata now becomes confusing, should we call them aggregator_metadata vs model_metadata, or maybe in_metadata vs out_metadata?

self.name = "custom"
self.code_set = False

def get_model_metadata(self):
Copy link
Member

Choose a reason for hiding this comment

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

this I don't understand why it's in the aggregator? Why not include model_metadata as an arg in APIClient.start_session()?

@Wrede Wrede merged commit 6462978 into master Oct 31, 2024
19 checks passed
@Wrede Wrede deleted the feature/SK-946 branch October 31, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants