Fix Ollama test script: resolve port conflicts and add dynamic CUDA version detection #1323
Conversation
…ersion detection - Fix process management: use proper PID extraction (column 2 instead of 1) - Add dynamic port detection: automatically find available ports starting from 11435 - Add dynamic CUDA version detection: automatically detect CUDA version from nvidia-smi/nvcc - Remove systemctl dependencies: make script container-safe - Improve error handling: better process verification and cleanup - Fix CUDA library selection: use detected version instead of undefined variable - Add robust port conflict resolution: handles cases where multiple ports are occupied
There was a problem hiding this comment.
Pull Request Overview
This PR fixes critical issues in the Ollama test script that were causing failures when Ollama was already installed on the host system. The changes implement dynamic port detection to avoid conflicts and add automatic CUDA version detection.
- Implements dynamic port allocation starting from 11435 to avoid conflicts with existing Ollama instances
- Adds automatic CUDA version detection from nvidia-smi/nvcc instead of relying on hardcoded values
- Improves process management with better PID detection and cleanup procedures
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if timeout 1 bash -c "echo >/dev/tcp/127.0.0.1/$port" 2>/dev/null; then | ||
| # Port is in use, try next | ||
| port=$((port + 1)) | ||
| else | ||
| # Port is available | ||
| echo $port | ||
| return 0 |
There was a problem hiding this comment.
The port availability check is incorrect. This bash redirection will succeed when the port is in use (connection established), but the logic treats success as 'port in use'. The condition should be inverted - if the connection succeeds, the port is occupied.
| if timeout 1 bash -c "echo >/dev/tcp/127.0.0.1/$port" 2>/dev/null; then | |
| # Port is in use, try next | |
| port=$((port + 1)) | |
| else | |
| # Port is available | |
| echo $port | |
| return 0 | |
| if ! timeout 1 bash -c "echo >/dev/tcp/127.0.0.1/$port" 2>/dev/null; then | |
| # Port is available | |
| echo $port | |
| return 0 | |
| else | |
| # Port is in use, try next | |
| port=$((port + 1)) |
| OLLAMA_PID=$(ps -ef | grep 'ollama serve' | grep -v grep | awk '{ print $2 }') | ||
|
|
||
| if [ -n "$OLLAMA_PID" ]; then | ||
| echo "Stopping existing ollama process: $OLLAMA_PID" | ||
| kill $OLLAMA_PID |
There was a problem hiding this comment.
This will only capture the first PID if multiple ollama processes are running. Consider using pgrep -f 'ollama serve' for more reliable process detection, or handle multiple PIDs appropriately.
There was a problem hiding this comment.
@tokk-nv what do you think? Sounds more efficient and clean, no?
2 participants
Problem
The Ollama test script was failing when Ollama was natively installed on the host system. This caused test failures due to:
Side Issues
Solution
This PR improves the Ollama test script with:
Testing
Impact