Add support for ${MODEL_ID} macro#226
Conversation
WalkthroughAdds a reserved Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ConfigLoader
participant MacroExpander
participant ModelProcessor
User->>ConfigLoader: Load config with macros + models
ConfigLoader->>MacroExpander: Expand global/user macros
MacroExpander-->>ConfigLoader: Global macros expanded
ConfigLoader->>ModelProcessor: For each model, start per-model processing
Note right of ModelProcessor: New step — substitute `${MODEL_ID}` in Cmd & CmdStop
ModelProcessor->>ModelProcessor: Replace `${MODEL_ID}` → modelId
ModelProcessor->>ModelProcessor: Validate remaining macros (e.g., `${PORT}`)
ModelProcessor-->>ConfigLoader: Return expanded per-model fields
ConfigLoader-->>User: Provide sanitized, per-model config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config.example.yaml(3 hunks)proxy/config.go(3 hunks)proxy/config_test.go(1 hunks)
🔇 Additional comments (6)
proxy/config.go (1)
245-245: LGTM: MODEL_ID correctly added to reserved macros.Adding "MODEL_ID" to the reserved macro list prevents users from defining their own MODEL_ID macro, which is appropriate since this is now a built-in system macro.
config.example.yaml (4)
44-44: Excellent documentation addition.This line clearly explains that the model ID is available as the
${MODEL_ID}macro, which helps users understand this new feature.
134-134: Clear example documentation.The updated comment appropriately describes the combination of cmd, cmdStop, and ${MODEL_ID} usage.
138-138: Perfect example of MODEL_ID macro usage.Replacing the hardcoded container name with
${MODEL_ID}demonstrates the key benefit of this feature - eliminating redundancy and copy-paste errors.
150-150: Consistent macro usage in cmdStop.Using
${MODEL_ID}in the cmdStop command maintains consistency and shows how the macro works across different command fields.proxy/config_test.go (1)
444-483: Comprehensive test coverage for MODEL_ID macro functionality.This test thoroughly validates the MODEL_ID macro implementation:
✅ Tests basic MODEL_ID expansion in model commands
✅ Tests MODEL_ID expansion within user-defined macros
✅ Verifies PORT macro continues to work with incremental assignment
✅ Tests complex model IDs with special characters ("author/model:F16")
✅ Confirms that macro definitions in config.Macros remain unexpanded
✅ Validates both cmd and cmdStop field expansionThe test cases cover the key scenarios and edge cases effectively.
|
I would also be interested in this functionality, I often find myself repeating the model-id in e.g. the helps me to better adhere to the "DRY"-principle in my config. |
|
This is ready for review. I've rebased to main and updated the examples here. Thought not quite as DRY as intended, still an improvement! |
proxy/config.go
Outdated
| modelConfig.CmdStop = strings.ReplaceAll(modelConfig.CmdStop, "${MODEL_ID}", modelId) | ||
| modelConfig.Proxy = strings.ReplaceAll(modelConfig.Proxy, "${MODEL_ID}", modelId) | ||
| modelConfig.CheckEndpoint = strings.ReplaceAll(modelConfig.CheckEndpoint, "${MODEL_ID}", modelId) | ||
| modelConfig.Filters.StripParams = strings.ReplaceAll(modelConfig.Filters.StripParams, "${MODEL_ID}", modelId) |
There was a problem hiding this comment.
I don’t think we need to support model_id in stripParams, checkEndpoint and proxy because it’ll have limited usefulness. Unless there’s a clear use case I would bias towards just cmd and cmdStop.
There was a problem hiding this comment.
No, nothing immediate that wouldn't be better solved elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
config.example.yaml (4)
52-53: Clarify ${MODEL_ID} scope to match implementation (cmd/cmdStop only).Implementation expands ${MODEL_ID} only in cmd and cmdStop. Reword to avoid implying support in proxy/checkEndpoint/filters.
-# - the model's ID is available in the ${MODEL_ID} macro, also available in macros defined above. +# - the model's ID is available via ${MODEL_ID} for use in cmd and cmdStop +# - macros defined above may include ${MODEL_ID} if they ultimately expand into those fields
152-154: Fix grammar: “runtimes” and remove duplicate “a”.- # container run times like Docker and Podman can be used reliably with a - # a combination of cmd, cmdStop, and ${MODEL_ID} + # container runtimes like Docker and Podman can be used reliably with + # a combination of cmd, cmdStop, and ${MODEL_ID}
156-160: Warn that ${MODEL_ID} may be an invalid Docker container name.IDs containing “/” or “:” won’t be valid for --name; the example could mislead. Add a brief note.
cmd: | + # NOTE: If you use ${MODEL_ID} for --name, ensure your model IDs are valid Docker container + # names (letters, digits, '_', '.', '-'). IDs with '/' or ':' will fail. docker run --name ${MODEL_ID} --init --rm -p ${PORT}:8080 -v /mnt/nvme/models:/models ghcr.io/ggml-org/llama.cpp:server --model '/models/Qwen2.5-Coder-0.5B-Instruct-Q4_K_M.gguf'
171-171: Mirror the Docker-name caveat for cmdStop.- cmdStop: docker stop ${MODEL_ID} + # NOTE: This will fail if ${MODEL_ID} is not a valid Docker container name. + cmdStop: docker stop ${MODEL_ID}proxy/config.go (1)
236-243: Keep the reserved-macro comment in sync (include MODEL_ID)./* check macro constraint rules: - name must fit the regex ^[a-zA-Z0-9_-]+$ - names must be less than 64 characters (no reason, just cause) - - name can not be any reserved macros: PORT + - name can not be any reserved macros: PORT, MODEL_ID - macro values must be less than 1024 characters */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
config.example.yaml(3 hunks)proxy/config.go(2 hunks)proxy/config_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- proxy/config_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: brettp
PR: mostlygeek/llama-swap#226
File: proxy/config.go:305-313
Timestamp: 2025-08-07T04:50:07.200Z
Learning: In the llama-swap project, model IDs are treated as trusted input from YAML configuration files. Additional validation for MODEL_ID macro expansion is not needed because if someone can control model IDs in the config, they can already inject malicious content in other fields like cmd. The configuration file is the trusted source.
📚 Learning: 2025-08-07T04:50:07.200Z
Learnt from: brettp
PR: mostlygeek/llama-swap#226
File: proxy/config.go:305-313
Timestamp: 2025-08-07T04:50:07.200Z
Learning: In the llama-swap project, model IDs are treated as trusted input from YAML configuration files. Additional validation for MODEL_ID macro expansion is not needed because if someone can control model IDs in the config, they can already inject malicious content in other fields like cmd. The configuration file is the trusted source.
Applied to files:
config.example.yamlproxy/config.go
🔇 Additional comments (1)
proxy/config.go (1)
254-258: LGTM: reserving MODEL_ID prevents user-defined collisions.
brettp
force-pushed
the
model_id_macro
branch
from
2e905de to
3f7827f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
config.example.yaml(3 hunks)proxy/config.go(3 hunks)proxy/config_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- proxy/config_test.go
- proxy/config.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: brettp
PR: mostlygeek/llama-swap#226
File: proxy/config.go:305-313
Timestamp: 2025-08-07T04:50:07.200Z
Learning: In the llama-swap project, model IDs are treated as trusted input from YAML configuration files. Additional validation for MODEL_ID macro expansion is not needed because if someone can control model IDs in the config, they can already inject malicious content in other fields like cmd. The configuration file is the trusted source.
📚 Learning: 2025-08-07T04:50:07.200Z
Learnt from: brettp
PR: mostlygeek/llama-swap#226
File: proxy/config.go:305-313
Timestamp: 2025-08-07T04:50:07.200Z
Learning: In the llama-swap project, model IDs are treated as trusted input from YAML configuration files. Additional validation for MODEL_ID macro expansion is not needed because if someone can control model IDs in the config, they can already inject malicious content in other fields like cmd. The configuration file is the trusted source.
Applied to files:
config.example.yaml
| # - required | ||
| # - each key is the model's ID, used in API requests | ||
| # - model settings have default values that are used if they are not defined here | ||
| # - the model's ID is available in the ${MODEL_ID} macro, also available in macros defined above |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify where ${MODEL_ID} expands (cmd/cmdStop only).
As implemented, ${MODEL_ID} is substituted in model.cmd and model.cmdStop (not in proxy or checkEndpoint). Update the comment to avoid implying broader scope.
Apply this diff:
-# - the model's ID is available in the ${MODEL_ID} macro, also available in macros defined above
+# - the model's ID is available as ${MODEL_ID} in model.cmd and model.cmdStop
+# - macros that are referenced by cmd/cmdStop may also include ${MODEL_ID}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # - the model's ID is available in the ${MODEL_ID} macro, also available in macros defined above | |
| # - the model's ID is available as ${MODEL_ID} in model.cmd and model.cmdStop | |
| # - macros that are referenced by cmd/cmdStop may also include ${MODEL_ID} |
🤖 Prompt for AI Agents
In config.example.yaml around line 52, the comment incorrectly implies
${MODEL_ID} is available broadly; clarify that ${MODEL_ID} is only substituted
into model.cmd and model.cmdStop. Update the comment text to state explicitly
that the ${MODEL_ID} macro is available only for model.cmd and model.cmdStop
(and not in proxy or checkEndpoint), so users won’t assume it expands elsewhere.
| # container runtimes like Docker and Podman can be used reliably with | ||
| # a combination of cmd, cmdStop, and ${MODEL_ID} | ||
| "docker-llama": |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid suggesting ${MODEL_ID} for container names; many model IDs are not Docker/Podman-safe.
Model IDs can contain '/', ':' (e.g., author/model:variant), which are invalid in container names. Recommend ${PORT} (numeric, unique per model using it) for names and stopping commands. This is a documentation/runtime correctness fix; MODEL_ID remains trusted input per project learnings.
Apply this diff:
- # container runtimes like Docker and Podman can be used reliably with
- # a combination of cmd, cmdStop, and ${MODEL_ID}
+ # container runtimes like Docker and Podman can be used with cmd/cmdStop.
+ # NOTE: prefer ${PORT} for container names; ${MODEL_ID} may contain '/' or ':' and won't be a valid container name.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # container runtimes like Docker and Podman can be used reliably with | |
| # a combination of cmd, cmdStop, and ${MODEL_ID} | |
| "docker-llama": | |
| # container runtimes like Docker and Podman can be used with cmd/cmdStop. | |
| # NOTE: prefer ${PORT} for container names; ${MODEL_ID} may contain '/' or ':' and won't be a valid container name. | |
| "docker-llama": |
🤖 Prompt for AI Agents
In config.example.yaml around lines 152 to 154, the docs currently suggest using
${MODEL_ID} in container names/stop commands which is unsafe because model IDs
may contain characters invalid for Docker/Podman; update the text and any
examples to recommend using ${PORT} (a numeric, unique per-model identifier) for
container names and stop commands instead, ensure guidance notes "${PORT} must
be unique per model instance and numeric", and adjust any example command
snippets to use ${PORT} in place of ${MODEL_ID}.
| proxy: "http://127.0.0.1:${PORT}" | ||
| cmd: | | ||
| docker run --name dockertest | ||
| docker run --name ${MODEL_ID} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a PORT-based container name to ensure validity across all model IDs.
Prevents failures when MODEL_ID contains invalid characters for container names.
Apply this diff:
- docker run --name ${MODEL_ID}
+ docker run --name llama-${PORT}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| docker run --name ${MODEL_ID} | |
| docker run --name llama-${PORT} |
🤖 Prompt for AI Agents
In config.example.yaml around line 157, the docker run command uses --name
${MODEL_ID} which can fail if MODEL_ID contains invalid container-name
characters; change the container name to use a PORT-based, always-valid
identifier (for example --name model_${PORT} or --name model-${PORT}) and update
any related references/env docs to use the PORT-based name so container naming
is deterministic and valid across all model IDs.
| # - on Windows, calls taskkill to stop the process | ||
| # - processes have 5 seconds to shutdown until forceful termination is attempted | ||
| cmdStop: docker stop dockertest | ||
| cmdStop: docker stop ${MODEL_ID} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align cmdStop with the PORT-based container name.
Keeps start/stop symmetric and robust to arbitrary model IDs.
Apply this diff:
- cmdStop: docker stop ${MODEL_ID}
+ cmdStop: docker stop llama-${PORT}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cmdStop: docker stop ${MODEL_ID} | |
| cmdStop: docker stop llama-${PORT} |
🤖 Prompt for AI Agents
In config.example.yaml at line 171, cmdStop currently uses docker stop
${MODEL_ID}; change it to stop the container using the same PORT-based container
name pattern used by cmdStart so start/stop are symmetric—replace ${MODEL_ID}
with the exact PORT-based container name expression used in cmdStart (for
example ${PORT}_${MODEL_ID} or whatever pattern is used elsewhere in this file).
|
@brettp looks good! Thank you for the contribution. |
3 participants
This adds
${MODEL_ID}support in commands and user-defined macros to simplify config. For example, this common config for docker:...can now be simplified to be more readable and less prone to copy and paste errors:
Summary by CodeRabbit
Documentation
${MODEL_ID}usage in macros and examples (updated docker-llama example and notes).New Features
${MODEL_ID}now expands in per-model commands (cmd/cmdStop) so container commands are tied to each model.Bug Fixes
MODEL_IDreserved to avoid conflicts with user-defined macros.Tests
${MODEL_ID}and${PORT}expand correctly and produce expected per-model commands.