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(docker) Add ServerApps docs #3439

Merged
merged 6 commits into from
May 16, 2024
Merged

docs(docker) Add ServerApps docs #3439

merged 6 commits into from
May 16, 2024

Conversation

Robert-Steiner
Copy link
Member

@Robert-Steiner Robert-Steiner commented May 13, 2024

Issue

Description

  • Adds documentation for the ServerApp.

Related issues/PRs

Proposal

Explanation

Checklist

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

Changelog entry

Any other comments?

@Robert-Steiner Robert-Steiner force-pushed the doc/serverapp-docker-docs branch 2 times, most recently from 3895c65 to 9c2fea8 Compare May 14, 2024 11:30
Signed-off-by: Robert Steiner <robert@flower.ai>
Comment on lines 185 to 187
If the requirement.txt contains the `flwr <https://pypi.org/project/flwr/>`__ or
`flwr-nightly <https://pypi.org/project/flwr-nightly/>`_ package, please ensure the version in
requirement.txt matches the docker image version.
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, we can skip installing flwr as long as the correct Docker image is used. Maybe we can rephrase as:

⚠️ Note that flwr is already installed in the flwr/supernode base image, so you only need to include other package dependencies in your requirements.txt, such as torch, tensorflow, etc ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the way you phrased it sounds better. I will go with that. The reason I included the note is because I'm afraid users develop locally with a requirements.txt containing the flwr package and once they're done they just copy the same file into the Docker image.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see why that can be the case. Since this is a self-contained "How-to" page, I think it's important to start from the simplest setup. This basic Dockerfile can then be expanded by users for other more complicated server setups.

Comment on lines 271 to 272
A key difference is the additional argument in the ``ENTRYPOINT`` command of the ServerApp
Dockerfile.
Copy link
Contributor

Choose a reason for hiding this comment

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

"A key difference is the different argument in the ENTRYPOINT command of the ServerApp Dockerfile."

The server:app argument can be passed in the same way as the client:app argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it.

doc/source/how-to-run-flower-using-docker.rst Show resolved Hide resolved
.
├── server.py # ServerApp code
├── task.py # ServerApp code
├── requirements.txt # ServerApp dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, ServerApp does not need any other packages other than flwr. Let's remove requirements.txt as one of the required files in this tree.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the examples/app-pytorch I need therequirements.txt because the ServerApp code uses the pytorch package but we can create our own example. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's adapt the examples. In fact, the quickstart-pytorch example is a better example to base this "How-to" on. For context, the app-pytorch uses a server-side model parameter intialization - that's why PyTorch is a dependency for running the ServerApp. For the quickstart-pytorch, it is a client-side parameter initialization - so all ML-related computation is done in the ClientApp. Can you please update the references to quickstart-pytorch instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I didn't know that. That makes sense! I will update it to use the quickstart-pytorch example instead.

Comment on lines 306 to 307
COPY requirements.txt .
RUN python -m pip install -U --no-cache-dir -r requirements.txt && pyenv rehash
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove requirements.txt and the installation of it in the ServerApp's Dockerfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 👍

doc/source/how-to-run-flower-using-docker.rst Show resolved Hide resolved
doc/source/how-to-run-flower-using-docker.rst Show resolved Hide resolved
Comment on lines 321 to 335
.. important::

If the requirement.txt contains the `flwr <https://pypi.org/project/flwr/>`__ or
`flwr-nightly <https://pypi.org/project/flwr-nightly/>`_ package, please ensure the version in
requirement.txt matches the docker image version.

Stable:

- Docker image: ``serverapp:1.8.0``
- requirement.txt: ``flwr[simulation]==1.8.0``

Nightly:

- Docker image: ``serverapp:1.9.0.dev20240501``
- requirement.txt: ``flwr-nightly[simulation]==1.9.0.dev20240501``
Copy link
Contributor

Choose a reason for hiding this comment

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

Since flwr is already installed in the base image, we can skip this step. Let's remove these lines.


$ docker run --rm flwr_serverapp:0.0.1 \
--insecure \
--server 192.168.1.100:9091
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we adopt the Docker network approach here (and also for SuperLink and SuperNode)? I think it will be clearer overall since we can just use the --name instead of an IP address --server.

docker run \
  --network flwr-net \
  --rm flwr/serverapp:1.8.0 \
  --insecure --server flwr-superlink:9091

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure since this only works if all containers are running on the same machine. We could add a note wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true. Maybe something like the following?

💡 To test running Flower locally, use the --network argument and pass the name of the Docker network to run your ServerApps.

Or did you have another comment in mind?

Copy link
Member Author

Choose a reason for hiding this comment

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

I took your comment and added a link to the Docker documentation on creating a bridge network.


.. code-block:: bash

$ docker run --rm -v ./ca.crt:/app/ca.crt flwr_serverapp:0.0.1 client:app \
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the full argument for consistency: docker run --rm --volume ...

Signed-off-by: Robert Steiner <robert@flower.ai>
Signed-off-by: Robert Steiner <robert@flower.ai>
@@ -117,7 +123,7 @@ building your own SuperNode image.
day. To ensure the versions are in sync, using the concrete tag, e.g., ``1.9.0.dev20240501``
instead of ``nightly`` is recommended.

We will use the ``app-pytorch`` example, which you can find in
We will use the ``quickstart-pytorch`` example, which you can find in
the Flower repository, to illustrate how you can dockerize your client-app.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename client-app > ClientApp for consistency.

@@ -126,14 +132,14 @@ Prerequisites
~~~~~~~~~~~~~

Before we can start, we need to meet a few prerequisites in our local development environment.
You can skip the first part if you want to run your client-app instead of the ``app-pytorch``
You can skip the first part if you want to run your client-app instead of the ``quickstart-pytorch``
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename client-app > ClientApp for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

Signed-off-by: Robert Steiner <robert@flower.ai>
Copy link
Contributor

@chongshenng chongshenng left a comment

Choose a reason for hiding this comment

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

LGTM! I only have minor formatting comments.

example.

#. Clone the flower repository.

.. code-block:: bash

$ git clone https://github.com/adap/flower.git && cd flower/examples/app-pytorch
$ git clone https://github.com/adap/flower.git && cd flower/examples/quickstart-pytorch
Copy link
Contributor

Choose a reason for hiding this comment

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

- git clone https://github.com/adap/flower.git && cd flower/examples/quickstart-pytorch
+ git clone --depth=1 https://github.com/adap/flower.git && cd flower/examples/quickstart-pytorch

@@ -126,14 +132,14 @@ Prerequisites
~~~~~~~~~~~~~

Before we can start, we need to meet a few prerequisites in our local development environment.
You can skip the first part if you want to run your client-app instead of the ``app-pytorch``
You can skip the first part if you want to run your client-app instead of the ``quickstart-pytorch``
example.

#. Clone the flower repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

flower > Flower

Comment on lines 176 to 177
base image, so you only need to include other package dependencies in your requirements.txt,
such as torch, tensorflow, etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

... in your requirements.txt such as torch, tensorflow, etc.


- Docker image: ``supernode:1.9.0.dev20240501``
- requirement.txt: ``flwr-nightly[simulation]==1.9.0.dev20240501``
we copy the ClientApp code into the image and set the entry point to ``flower-client-app`` with
Copy link
Contributor

Choose a reason for hiding this comment

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

we copy the client.py module into the image ...


- Docker image: ``serverapp:1.9.0.dev20240501``
- requirement.txt: ``flwr-nightly[simulation]==1.9.0.dev20240501``
executed in the ``/app`` directory. In the last two lines, we copy the ServerApp code into the
Copy link
Contributor

Choose a reason for hiding this comment

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

..., we copy the ServerApp module into the ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we call it ServerApp or server.py to be consistent with change above?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right @Robert-Steiner! Let's stick to server.py.

..., we copy the server.py module into the ...

Signed-off-by: Robert Steiner <robert@flower.ai>
Copy link
Contributor

@chongshenng chongshenng left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Robert-Steiner!

@Robert-Steiner Robert-Steiner marked this pull request as ready for review May 16, 2024 08:17
@tanertopal tanertopal enabled auto-merge (squash) May 16, 2024 14:43
Copy link
Member

@tanertopal tanertopal left a comment

Choose a reason for hiding this comment

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

lgtm!

@tanertopal tanertopal merged commit 81c08fd into main May 16, 2024
28 checks passed
@tanertopal tanertopal deleted the doc/serverapp-docker-docs branch May 16, 2024 14:48
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.

3 participants