Add aiDAPTIVCache applications documents - #67
Conversation
Summary of ChangesHello @tsaishawn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces essential documentation for leveraging Phison aiDAPTIVCache devices within the OtterScale Kubernetes platform. It provides detailed guides for installing the foundational aiDAPTIV Cache Operator, configuring and executing LLM fine-tuning jobs, and deploying high-throughput LLM inference services, thereby empowering users to effectively integrate and manage AI workloads with specialized hardware acceleration. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 adds documentation for aiDAPTIVCache applications. The guides are comprehensive, but I've found a few areas for improvement. My feedback includes corrections for typos and inconsistencies in the instructions which could confuse users. I've also pointed out a security risk in the script examples that use sshpass with plaintext passwords and suggested safer alternatives. Fixing these issues will enhance the quality and clarity of the documentation.
| sshpass -p 'your-password' scp -o StrictHostKeyChecking=no -r \ | ||
| user@remote-host:/path/to/your-model /mnt/data/models/ |
There was a problem hiding this comment.
Using sshpass -p 'your-password' is insecure as it exposes the password in the process list and potentially in logs. It's strongly recommended to use SSH keys for authentication instead. If passwords must be used, please add a prominent warning about the security risks.
| sshpass -p 'your-password' scp -o StrictHostKeyChecking=no -r \ | ||
| user@remote-host:/path/to/your-model /mnt/data/models/ |
There was a problem hiding this comment.
Using sshpass -p 'your-password' is insecure as it exposes the password in the process list and potentially in logs. It's strongly recommended to use SSH keys for authentication instead. If passwords must be used, please add a prominent warning about the security risks.
|
|
||
| 3. **Install the Operator** | ||
|
|
||
| - Find `aidaptivcache-inference` in the Store list |
There was a problem hiding this comment.
There seems to be a typo in the chart name. The user is instructed to find aidaptivcache-inference in the store list, but this document is about installing the aidaptivcache-operator. The imported chart is also aidaptivcache-operator. This should likely be aidaptivcache-operator.
- Find `aidaptivcache-operator` in the Store list
| You still need an NFS mount for storing training outputs. Add NFS mount commands after the SCP section: | ||
|
|
||
| ```yaml | ||
| postscript: | | ||
| # Install tools | ||
| apt-get update && apt-get install -y sshpass nfs-common | ||
|
|
||
| # Copy model via SCP | ||
| mkdir -p /mnt/data/models | ||
| echo "Copying model from remote server..." | ||
| sshpass -p 'your-password' scp -o StrictHostKeyChecking=no -r \ | ||
| user@remote-host:/path/to/model /mnt/data/models/ | ||
|
|
||
| # Mount NFS for output storage | ||
| mkdir -p /mnt/data/output | ||
| echo "Mounting NFS for output storage..." | ||
| mount -t nfs4 -o nfsvers=4.1 10.102.197.0:/volumes/_nogroup/output-path /mnt/data/output | ||
|
|
||
| echo "Setup complete!" | ||
| ``` |
There was a problem hiding this comment.
This section is confusing. It says "Add NFS mount commands after the SCP section" but then provides a postscript example that repeats the apt-get install and scp commands from the prescript example above. It seems the intention was to show a combined prescript that handles both SCP for input and NFS for output. Presenting this as a postscript is misleading and duplicates effort.
Please clarify whether this is meant to be a postscript or an alternative, more complete prescript. If it's the latter, it should be presented as such. For example:
"Here is a complete prescript example that copies the model via SCP and mounts an NFS volume for outputs:"
prescript: |
# Install tools
apt-get update && apt-get install -y sshpass nfs-common
# Copy model via SCP
mkdir -p /mnt/data/models
echo "Copying model from remote server..."
sshpass -p 'your-password' scp -o StrictHostKeyChecking=no -r \
user@remote-host:/path/to/model /mnt/data/models/
# Mount NFS for output storage
mkdir -p /mnt/data/output
echo "Mounting NFS for output storage..."
mount -t nfs4 -o nfsvers=4.1 10.102.197.0:/volumes/_nogroup/output-path /mnt/data/output
echo "Setup complete!"| postscript: | | ||
| echo "Training job completed" | ||
| echo "Model saved to: $outputDir" |
There was a problem hiding this comment.
The variable $outputDir is used here, but it's unlikely to be defined in the shell environment where this postscript is executed. This variable is defined in the Helm values.yaml but is not automatically exported as an environment variable. This command will likely fail or print an empty string for $outputDir.
To fix this, you should either hardcode the path (e.g., /mnt/data/output, which is the value from envConfig.pathSettings.outputDir) or ensure the Helm chart is configured to pass this value as an environment variable to the container.
postscript: |
echo "Training job completed"
echo "Model saved to: /mnt/data/output"
| port: 8000 | ||
| gpuMemoryUtilization: 0.9 | ||
| maxModelLen: 32768 | ||
| tensorParallelSize: 4 |
There was a problem hiding this comment.
The example configuration for vllm.args shows tensorParallelSize: 4, which implies a multi-GPU setup. However, the resource configuration examples later in the document (e.g., lines 363-372 and the complete example starting at line 389) are configured for a single GPU (otterscale.com/vgpu: 1).
This is inconsistent and could confuse users. The Aside on line 382 correctly states that tensorParallelSize must match the vgpu count.
To improve clarity, please make the examples consistent. I suggest changing tensorParallelSize to 1 in this example to match the single-GPU setup demonstrated elsewhere in the document.
tensorParallelSize: 1
1 participant
No description provided.