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

Support CreateNode and DeleteNode in REST API #2283

Merged
merged 24 commits into from
Sep 11, 2023
Merged

Conversation

cnxdeveloper
Copy link
Contributor

Issue

By default, in restful API mode, Flower only supports clients running as anonymous. This makes it difficult to manage tasks and results for each client.

Description

Related issues/PRs

Proposal

  • Update the 2 POST API requests to create and delete a node.
  • Update the client code to support calling the API to create a node and delete a node.
  • Update the code and README in flower/examples/mt-pytorch to run and test with restful mode.

Explanation

Checklist

  • Implement proposed change
  • Write tests
  • Update documentation
  • Update changelog
  • Make CI checks pass
  • Ping maintainers on Slack (channel #contributions)

Any other comments?

@danieljanes
Copy link
Member

Thanks for the well-scoped PR @cnxdeveloper! It looks pretty clean and easy to review, I will try to do a full review by the end of this week. In the meantime, could you already add an entry to the changelog in the Unreleased section?

@cnxdeveloper
Copy link
Contributor Author

Thanks for the well-scoped PR @cnxdeveloper! It looks pretty clean and easy to review, I will try to do a full review by the end of this week. In the meantime, could you already add an entry to the changelog in the Unreleased section?

Hi @danieljanes,
I really like the Flower framework. It's easy to use and scale up. Thank you for creating such an awesome repo!
I haven't updated the changelog in the Unreleased section yet. I'll do that later today.

Copy link
Member

@danieljanes danieljanes left a comment

Choose a reason for hiding this comment

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

The PR is in a good shape 👍 I added some comments to align the coding style with the rest of the codebase

examples/mt-pytorch/client.py Outdated Show resolved Hide resolved
src/py/flwr/client/rest_client/connection.py Outdated Show resolved Hide resolved
doc/source/ref-changelog.md Outdated Show resolved Hide resolved
examples/mt-pytorch/README.md Outdated Show resolved Hide resolved
examples/mt-pytorch/README.md Outdated Show resolved Hide resolved
src/py/flwr/server/fleet/rest_rere/rest_api.py Outdated Show resolved Hide resolved
src/py/flwr/client/rest_client/connection.py Outdated Show resolved Hide resolved
src/py/flwr/client/rest_client/connection.py Outdated Show resolved Hide resolved
Copy link
Member

@danieljanes danieljanes 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 fast updates, @cnxdeveloper! I added a few comments to improve consistency and I think that we can merge the PR once they are resolved.

examples/mt-pytorch/client.py Outdated Show resolved Hide resolved
examples/mt-pytorch/client.py Outdated Show resolved Hide resolved
src/py/flwr/client/rest_client/connection.py Outdated Show resolved Hide resolved
src/py/flwr/client/rest_client/connection.py Outdated Show resolved Hide resolved
src/py/flwr/client/rest_client/connection.py Outdated Show resolved Hide resolved
src/py/flwr/client/rest_client/connection.py Show resolved Hide resolved
src/py/flwr/client/rest_client/connection.py Show resolved Hide resolved
src/py/flwr/server/fleet/rest_rere/rest_api.py Outdated Show resolved Hide resolved
src/py/flwr/server/fleet/rest_rere/rest_api.py Outdated Show resolved Hide resolved
src/py/flwr/server/fleet/rest_rere/rest_api.py Outdated Show resolved Hide resolved
cnxdeveloper and others added 3 commits September 8, 2023 17:44
Co-authored-by: Daniel J. Beutel <daniel@flower.dev>
Co-authored-by: Daniel J. Beutel <daniel@flower.dev>
@danieljanes
Copy link
Member

@cnxdeveloper there's a formatting error: https://github.com/adap/flower/actions/runs/6120808796/job/16614584948?pr=2283

This can be fixed by running ./dev/format.sh

@danieljanes danieljanes changed the title Implement a RESTful API for Flower that supports creating and deleting nodes. Support CreateNode and DeleteNode in REST API Sep 8, 2023
@cnxdeveloper
Copy link
Contributor Author

@cnxdeveloper there's a formatting error: https://github.com/adap/flower/actions/runs/6120808796/job/16614584948?pr=2283

This can be fixed by running ./dev/format.sh

Hi @danieljanes I fixed code format by script ./dev/format.sh

Copy link
Member

@danieljanes danieljanes left a comment

Choose a reason for hiding this comment

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

This should resolve the CI error

src/py/flwr/client/rest_client/connection.py Outdated Show resolved Hide resolved
@danieljanes
Copy link
Member

I fixed a few linter warnings, could you reformat once more @cnxdeveloper?

@danieljanes
Copy link
Member

Almost there! Only formatting issues in two markdown files left: https://github.com/adap/flower/actions/runs/6138546228/job/16655523326?pr=2283#step:6:252

@cnxdeveloper
Copy link
Contributor Author

cnxdeveloper commented Sep 11, 2023

Almost there! Only formatting issues in two markdown files left: https://github.com/adap/flower/actions/runs/6138546228/job/16655523326?pr=2283#step:6:252

Hi I tried to use format script to reformat these markdown files.

Copy link
Member

@danieljanes danieljanes left a comment

Choose a reason for hiding this comment

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

Lgtm, merging! Thanks for the contribution @cnxdeveloper 🎉

@danieljanes danieljanes merged commit dc1730a into adap:main Sep 11, 2023
26 checks passed
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.

2 participants