updated nvflare-docker-run.sh and drive_admin_api.py - #98
Conversation
…provements as well as support for nvflare 2.6 and 2.7. Also added a markdown file for usage
|
Cool! Take a look at the conflicts with the main branch. Also what about NVFlare v2.8? |
tal-rhino
left a comment
tal-rhino
left a comment
There was a problem hiding this comment.
Looks excellent overall! A few comments and suggestions.
| ## Prerequisites | ||
|
|
||
| - Docker | ||
| - **macOS only:** GNU coreutils (`brew install coreutils`) |
There was a problem hiding this comment.
What is this needed for? It wasn't needed in the past, would be nice not to require users to install this.
There was a problem hiding this comment.
I'm assuming this is mostly bring in changes from the similar script in the fcp repo?
|
|
||
| ## Installation (Recommended) | ||
|
|
||
| Copy both scripts to `/usr/local/bin/`, dropping the `.sh` extension from the main script, and make it executable: |
There was a problem hiding this comment.
Nice! But why drop the .sh? I think that could cause more confusion than benefit.
| ```bash | ||
| cp nvflare-docker-run.sh /usr/local/bin/nvflare-docker-run | ||
| cp drive_admin_api.py /usr/local/bin/drive_admin_api.py | ||
| chmod +x /usr/local/bin/nvflare-docker-run |
There was a problem hiding this comment.
both scripts should have chmod +x
| Verify the script is installed and executable: | ||
|
|
||
| ```bash | ||
| which nvflare-docker-run |
There was a problem hiding this comment.
Better to use command -v rather than which, the former is more standard and likely to be available on all systems.
| Verify that `/usr/local/bin/` is in your PATH: | ||
|
|
||
| ```bash | ||
| echo $PATH | ||
| ``` | ||
|
|
||
| You should see `/usr/local/bin` in the output. If not, add it to your shell profile (e.g. `~/.bashrc` or `~/.zshrc`): | ||
|
|
||
| ```bash | ||
| # For zsh (default on macOS): | ||
| echo 'export PATH="/usr/local/bin:$PATH"' >> ~/.zshrc | ||
| source ~/.zshrc | ||
|
|
||
| # For bash (default on Ubuntu): | ||
| echo 'export PATH="/usr/local/bin:$PATH"' >> ~/.bashrc | ||
| source ~/.bashrc | ||
| ``` |
There was a problem hiding this comment.
I think this is excessive here, since not having /usr/local/bin would be extremely strange. If you want to keep this, consider putting it in the "troubleshooting" section with a reference below.
|
|
||
| This should return `/usr/local/bin/nvflare-docker-run`. | ||
|
|
||
| Alternatively, if you prefer to keep the files in your project directory, you can run the script directly without installation: |
There was a problem hiding this comment.
This requires chmod +x first.
| echo ' --platform PLATFORM Platform to build container for, e.g. "linux/amd64",' | ||
| echo ' via `docker build --platform=PLATFORM`' | ||
| echo " --n-clients NUM Number of clients to run. (default: $n_clients)" | ||
| echo " --app_name NAME Name of the NVFlare app directory. (default: app)" |
There was a problem hiding this comment.
Do we support using a different name on FCP?
| # Check that each dataset subdirectory contains a dataset.csv. | ||
| for dataset_dir in "$site_dir"/*/; do | ||
| if [ ! -f "$dataset_dir/dataset.csv" ]; then | ||
| input_errors+=(" site-$clientnum: missing dataset.csv in $dataset_dir") | ||
| fi | ||
| done |
There was a problem hiding this comment.
We now also support dataset.parquet.
2 participants
updated nvflare-docker-run.sh and drive_admin_api.py with some QOL improvements as well as support for nvflare 2.6 and 2.7. Also added a markdown file for usage