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

docs(framework) Add how-to run flwr with deployment engine guide #4372

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Robert-Steiner
Copy link
Member

Issue

Description

Related issues/PRs

Proposal

Explanation

Checklist

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

Any other comments?

Signed-off-by: Robert Steiner <robert@flower.ai>
@Robert-Steiner Robert-Steiner self-assigned this Oct 25, 2024
@Robert-Steiner Robert-Steiner changed the title docs(framework) Add How to run flwr with Deployment engine guide docs(framework) Add how-to run flwr with deployment engine guide Oct 25, 2024
Copy link
Contributor

@jafermarq jafermarq left a comment

Choose a reason for hiding this comment

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

Hey @Robert-Steiner , i like the flow. Some comments below. I'm thinking to merge this as soon as we release 1.13.0 which will have fused SuperLink and SuperExec.

In this how-to guide, you will:

- Set up a Flower project from scratch using the PyTorch template.
- Run a Flower using the Deployment Engine.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking we can remove this step?

doc/source/how-to-run-flower-with-deployment-engine.rst Outdated Show resolved Hide resolved
Comment on lines +83 to +84
* | ``--node-config "partition-id=0 num-partitions=2"``: Set the partition ID to ``0`` and the
| number of partitions to ``2`` for the SuperNode configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* | ``--node-config "partition-id=0 num-partitions=2"``: Set the partition ID to ``0`` and the
| number of partitions to ``2`` for the SuperNode configuration.
* | ``--node-config "partition-id=0 num-partitions=2"``: Set the partition ID to ``0`` and the
| number of partitions to ``2`` for the SuperNode configuration. This is needed because the
| PyTorch template used was designed with the Simulation Engine in mind. In a real-world setup using the Deployment Engine, you'd instead pass in :code:`--node-config` the path to your local dataset.

Copy link
Contributor

Choose a reason for hiding this comment

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

i thought of adding a bit more context. Just to explain that those partition-id and num-partitions keys are just there because the ClientApp in this particular template needs them. A normal distributed setup would likely use other keys... do you think there's a clearer way of saying this?

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense to me. We could also use the PyTorch Quickstart example to provide a more realistic use case. wdyt?

Comment on lines +98 to +99
--isolation subprocess

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
--isolation subprocess
--isolation subprocess
.. dropdown:: Understand the command
* ``--supernode-address 127.0.0.1:9094``: Note how we indicate a different port. This is because if you run two `SuperNodes` on the same machine, they can't make use of the same port to communicate with the sub-process executing the `ClientApp`.

Copy link
Contributor

Choose a reason for hiding this comment

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

another option would be to skip showing this second flower-supernode command and say: "if you run in a different machine, run exactly as before. If on the same machine, just change the port". This sounds like a better way to me but then the question is: should we present the --superlink IPs in a more general way instead of assuming them being localhost?

Copy link
Member Author

Choose a reason for hiding this comment

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

What about using a tab component and show both versions (localhost, different host)?

Copy link
Contributor

@jafermarq jafermarq left a comment

Choose a reason for hiding this comment

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

Hey @Robert-Steiner , i like the flow. Some comments below. I'm thinking to merge this as soon as we release 1.13.0 which will have fused SuperLink and SuperExec.

@jafermarq jafermarq added the documentation Improvements or additions to documentation label Oct 25, 2024
Co-authored-by: Javier <jafermarq@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants