fix: update README for new impl.#438
Conversation
Signed-off-by: Klaus Ma <klausm@nvidia.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the README to include a new performance benchmark section and simplifies the local installation and multi-node deployment instructions using the flmadm tool. A review comment suggests adding a step to source the environment script for local installations to ensure binaries are correctly added to the system PATH.
| ### Verify the Installation | ||
|
|
||
| Then, verify the installation with `flmping` in the pod. Additionally, you can explore more meaningful examples [here](examples): | ||
| After starting Flame (via either option), verify the installation with `flmping`: |
There was a problem hiding this comment.
For users who choose Option 2 (Local Installation), the binaries are installed to /usr/local/flame/bin by default. These will not be in the system PATH automatically. It is recommended to mention that users should source the environment script or update their PATH before running the verification commands to avoid "command not found" errors.
| After starting Flame (via either option), verify the installation with `flmping`: | |
| After starting Flame (via either option), verify the installation with `flmping`. For local installations, you may first need to source the environment script: `source /usr/local/flame/sbin/flmenv.sh`. |
There was a problem hiding this comment.
Code Review
This pull request updates the README.md to include a performance benchmark section, streamlines the local installation process using flmadm, and provides clearer instructions for multi-node deployments. A review comment suggested adding a step to source the environment script after installation to ensure the CLI tools are available in the user's path.
| # Install all components from local source and start services | ||
| $ sudo flmadm install --all --src-dir . --enable |
There was a problem hiding this comment.
After running flmadm install, the Flame binaries (such as flmping and flmctl) are installed to /usr/local/flame/bin by default. To use them in the current shell session, users need to source the generated environment script or manually update their PATH. Adding this step to the Quick Start guide will prevent "command not found" errors in the subsequent verification step.
| # Install all components from local source and start services | |
| $ sudo flmadm install --all --src-dir . --enable | |
| # Install all components from local source and start services | |
| $ sudo flmadm install --all --src-dir . --enable | |
| $ source /usr/local/flame/sbin/flmenv.sh |
There was a problem hiding this comment.
Code Review
This pull request updates the README.md to include a performance benchmark section and modernizes the local installation instructions to use the flmadm tool. It also clarifies multi-node deployment steps. Feedback suggests adding a prerequisite check for uv and informing users about updating their PATH or sourcing the environment script to access Flame binaries after installation.
| $ sudo cp target/release/flmadm /usr/local/bin/ | ||
|
|
||
| # Install all components from local source and start services | ||
| $ sudo flmadm install --all --src-dir . --enable |
There was a problem hiding this comment.
The flmadm install command with the --all flag (or --worker/--client) requires uv to be installed on the host system, as enforced in flmadm/src/commands/install.rs. Since this is a 'Quick Start' guide, it would be helpful to mention this prerequisite here to prevent users from encountering a validation error during their first installation attempt.
| $ sudo flmadm install --all --src-dir . --enable | |
| # Ensure uv is installed (e.g., curl -LsSf https://astral.sh/uv/install.sh | sh) | |
| $ sudo flmadm install --all --src-dir . --enable |
| ### Verify the Installation | ||
|
|
||
| Then, verify the installation with `flmping` in the pod. Additionally, you can explore more meaningful examples [here](examples): | ||
| After starting Flame (via either option), verify the installation with `flmping`: |
There was a problem hiding this comment.
After installation, the Flame binaries (like flmping and flmctl) are located in the installation prefix (defaulting to /usr/local/flame/bin). To run them as shown in the examples below, users will need to add this directory to their PATH or source the generated environment script (e.g., source /usr/local/flame/sbin/flmenv.sh). Adding a brief note about this would improve the user experience.
There was a problem hiding this comment.
Code Review
This pull request adds a performance benchmark section to the README and updates the installation guides to utilize the flmadm tool for both local and multi-node deployments. Feedback includes suggestions to provide hardware context for benchmarks, document the uv prerequisite for flmadm, and use the install utility for binary placement to ensure correct permissions.
|
|
||
| ## Performance | ||
|
|
||
| Flame is designed for high-throughput task execution. Here's a benchmark running 30,000 tasks: |
There was a problem hiding this comment.
Providing context about the test environment (e.g., hardware specifications or 'single-node deployment') where this benchmark was executed would help users understand the performance characteristics and set realistic expectations.
| ### Option 2: Local Installation (Faster for Development) | ||
| ### Option 2: Local Installation with flmadm (Faster for Development) | ||
|
|
||
| For development and testing, you can install Flame directly on your machine using `flmadm`: |
There was a problem hiding this comment.
The flmadm install command requires uv to be installed on the system to manage Python SDK components. Since the --all flag includes worker and client profiles, the installation will fail if uv is missing. It would be beneficial to mention this prerequisite here.
| For development and testing, you can install Flame directly on your machine using `flmadm`: | |
| For development and testing, you can install Flame directly on your machine using flmadm (requires [uv](https://astral.sh/uv/)): |
| $ /tmp/flame-dev/bin/flame-executor-manager --config /tmp/flame-dev/conf/flame-cluster.yaml & | ||
| # Build and install flmadm | ||
| $ cargo build --release -p flmadm | ||
| $ sudo cp target/release/flmadm /usr/local/bin/ |
There was a problem hiding this comment.
Using the install command is generally preferred over cp for installing binaries to system paths as it allows setting the execution bit and ownership in a single step.
| $ sudo cp target/release/flmadm /usr/local/bin/ | |
| $ sudo install -m 755 target/release/flmadm /usr/local/bin/ |
- Add single-node deployment context for benchmark - Add Rust and uv prerequisites for flmadm - Use install command instead of cp for binary - Add step to source flmenv.sh for PATH setup
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
No description provided.