Conversation
|
""" WalkthroughAutomatic port assignment for model processes was implemented by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigLoader
participant ModelProcess
User->>ConfigLoader: Provide config with models using ${PORT}
ConfigLoader->>ConfigLoader: Assign StartPort (default 5800 or user-defined)
ConfigLoader->>ConfigLoader: For each model with ${PORT}, assign incrementing port
ConfigLoader->>ModelProcess: Launch model with cmd/proxy using assigned port
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
proxy/config.go (1)
115-121: StartPort validation logic looks goodThe default port (5800) and validation logic are well implemented. However, the error message at line 120 mentions "start_port" while the YAML field is actually "startPort" (camelCase).
- return Config{}, fmt.Errorf("start_port must be greater than 1") + return Config{}, fmt.Errorf("startPort must be greater than 1")README.md (1)
195-195: Incomplete sentence in documentationLine 195 contains an incomplete sentence "llama-s" that seems to be cut off.
Either complete the sentence or remove it.
proxy/config_test.go (1)
317-345: Consider adding test for boundary port valuesWhile the current tests are good, consider adding a test case for very high port numbers to verify behavior when approaching system limits.
You could add a test case like:
t.Run("High port values", func(t *testing.T) { content := ` startPort: 65000 models: model1: cmd: svr --port ${PORT} model2: cmd: svr --port ${PORT} ` config, err := LoadConfigFromReader(strings.NewReader(content)) if !assert.NoError(t, err) { t.Fatalf("Failed to load config: %v", err) } assert.Equal(t, 65000, config.StartPort) assert.Equal(t, "svr --port 65000", config.Models["model1"].Cmd) assert.Equal(t, "svr --port 65001", config.Models["model2"].Cmd) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(3 hunks)proxy/config.go(4 hunks)proxy/config_test.go(2 hunks)
🔇 Additional comments (7)
proxy/config.go (1)
68-69: Good addition of StartPort field with clear purposeThe field is properly documented in the code with a descriptive comment and appropriate YAML tag.
README.md (4)
73-80: Clear documentation of the new automatic port featureThe explanation is concise and helpful, covering:
- How to use the ${PORT} placeholder
- Default values and configuration options
- When proxy can be omitted
This will make it easy for users to adopt this feature.
121-121: Good example of ${PORT} usage in commandThis updated example properly demonstrates the new placeholder functionality.
125-129: Comprehensive example showing ${PORT} in both proxy and Docker port mappingThis is a helpful example demonstrating the flexibility of the automatic port assignment feature with Docker containers.
94-95: Helpful comment about omitting proxy when using ${PORT}This clarifies an important aspect of the feature for users.
proxy/config_test.go (2)
77-77: Appropriate update to existing test caseThe test case was correctly updated to include the default StartPort value (5800).
284-347: Comprehensive test coverage for the automatic port assignment featureThe test suite is well-structured with separate test cases for:
- Default port value (5800)
- User-specified port values
- Invalid port configurations (non-numeric and negative values)
- Various automatic port assignment scenarios
The tests are thorough and cover the main functionality paths including:
- Default proxy generation when none is provided
- Replacement of ${PORT} in custom proxy URLs
- Verification that models without ${PORT} remain unchanged
This will help ensure the feature remains stable during future changes.
| // iterate over the models and replace any ${PORT} with the next available port | ||
| // Get and sort all model IDs first, makes testing more consistent | ||
| modelIds := make([]string, 0, len(config.Models)) | ||
| for modelId := range config.Models { | ||
| modelIds = append(modelIds, modelId) | ||
| } | ||
| sort.Strings(modelIds) // This guarantees stable iteration order | ||
|
|
||
| // iterate over the sorted models | ||
| nextPort := config.StartPort | ||
| for _, modelId := range modelIds { | ||
| modelConfig := config.Models[modelId] | ||
| if strings.Contains(modelConfig.Cmd, "${PORT}") { | ||
| modelConfig.Cmd = strings.ReplaceAll(modelConfig.Cmd, "${PORT}", strconv.Itoa(nextPort)) | ||
| if modelConfig.Proxy == "" { | ||
| modelConfig.Proxy = fmt.Sprintf("http://localhost:%d", nextPort) | ||
| } else { | ||
| modelConfig.Proxy = strings.ReplaceAll(modelConfig.Proxy, "${PORT}", strconv.Itoa(nextPort)) | ||
| } | ||
| nextPort++ | ||
| config.Models[modelId] = modelConfig | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Well-implemented automatic port assignment logic
The implementation nicely handles:
- Sorting model IDs for consistent port assignment
- Replacing ${PORT} placeholders in commands
- Setting default proxy or updating existing proxy URLs
- Incrementing ports sequentially
I appreciate the thoughtful approach to ensure stable port assignments by sorting model IDs.
One concern is potential port conflicts with system services. Let's check if there's any validation to ensure assigned ports are available:
🏁 Script executed:
#!/bin/bash
# Search for system port availability checks
rg -A 3 "port.*avail|avail.*port|CheckPort"Length of output: 347
Add port availability checks during automatic port assignment
The current logic in proxy/config.go (lines 134–156) sequentially assigns ports from StartPort without verifying they’re free, which may cause conflicts with existing services. Please integrate a check—e.g., attempt a temporary net.Listen on the candidate port and retry on error—before finalizing each assignment.
• File: proxy/config.go, lines 134–156
• Suggested approach:
for _, modelId := range modelIds {
// find next free port
for {
ln, err := net.Listen("tcp", fmt.Sprintf(":%d", nextPort))
if err != nil {
nextPort++
continue
}
ln.Close()
break
}
// replace ${PORT} and set proxy as before
...
nextPort++
}There was a problem hiding this comment.
this is fine it is up to the users to provide a good startPort for the range.
There was a problem hiding this comment.
@mostlygeek Understood! Thanks for clarifying the design decision. I'll respect your approach of letting users manage their port range selection.
✏️ Learnings added
Learnt from: mostlygeek
PR: mostlygeek/llama-swap#117
File: proxy/config.go:0-0
Timestamp: 2025-05-05T23:54:14.185Z
Learning: In the llama-swap project, automatic port assignment does not include port availability checks. It's the user's responsibility to provide an appropriate startPort that doesn't conflict with other services.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
proxy/config.go (1)
134-158: Add port availability checks during automatic port assignmentThe current logic sequentially assigns ports from
StartPortwithout verifying they're free, which may cause conflicts with existing services.Please integrate a check (e.g., attempt a temporary
net.Listenon the candidate port and retry on error) before finalizing each assignment, as suggested in the previous review comment.Additional imports needed:
import "net"Example implementation:
for _, modelId := range modelIds { // find next free port for { ln, err := net.Listen("tcp", fmt.Sprintf(":%d", nextPort)) if err != nil { nextPort++ continue } ln.Close() break } // replace ${PORT} and set proxy as before modelConfig := config.Models[modelId] if strings.Contains(modelConfig.Cmd, "${PORT}") { modelConfig.Cmd = strings.ReplaceAll(modelConfig.Cmd, "${PORT}", strconv.Itoa(nextPort)) if modelConfig.Proxy == "" { modelConfig.Proxy = fmt.Sprintf("http://localhost:%d", nextPort) } else { modelConfig.Proxy = strings.ReplaceAll(modelConfig.Proxy, "${PORT}", strconv.Itoa(nextPort)) } nextPort++ config.Models[modelId] = modelConfig } else if modelConfig.Proxy == "" { return Config{}, fmt.Errorf("model %s requires a proxy value when not using automatic ${PORT}", modelId) } }
🧹 Nitpick comments (2)
proxy/config.go (2)
115-121: Improve validation for port rangesThe validation for
StartPortensures it's greater than 1, which is good, but could be enhanced to handle additional constraints:
- Ports ≤ 1024 are privileged on Unix systems and might require elevated permissions
- No upper bound check to ensure ports don't exceed the maximum valid port number (65535)
Consider implementing a more comprehensive validation:
if config.StartPort == 0 { // default to 5800 config.StartPort = 5800 } else if config.StartPort < 1 { return Config{}, fmt.Errorf("start_port must be greater than 1") +} else if config.StartPort <= 1024 { + // Warning: using system ports +} else if config.StartPort > 65535 { + return Config{}, fmt.Errorf("startPort must be less than or equal to 65535") }
120-120: Fix error message to match configuration field nameThe error message refers to
start_portwith an underscore, but the YAML field isstartPort(camelCase).- return Config{}, fmt.Errorf("start_port must be greater than 1") + return Config{}, fmt.Errorf("startPort must be greater than 1")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proxy/config.go(4 hunks)proxy/config_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- proxy/config_test.go
🔇 Additional comments (2)
proxy/config.go (2)
140-140: Well-implemented sorting for stable port assignmentsSorting the model IDs ensures consistent port assignments across application restarts, which is excellent for stability and testing.
147-152: Well-designed port substitution logicThe implementation correctly handles:
- Replacing ${PORT} placeholders in commands
- Setting default proxy URL when none is provided
- Updating existing proxy URLs that contain ${PORT}
1 participant
Fixes #105
${PORT}inmodel.cmdwill automatically be replaced with a internally managed port numbersconfig.startPort, which defaults to 5800model.proxy, if omitted will automatically be assignedhttp://localhost:${PORT}Summary by CodeRabbit
Documentation
New Features
Tests