Add macros to Configuration schema #149
Merged
+190
−149
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Walkthrough
A macro substitution mechanism was introduced to the configuration system, allowing user-defined macros in model configuration fields. The `Config` struct now includes a `Macros` map, and macro validation, substitution, and error handling were added to the config loader. Corresponding tests were implemented to verify correct macro expansion and error reporting. The README was updated to simplify configuration examples and point to external documentation.
## Changes
| File(s) | Change Summary |
|---------------------------------|----------------------------------------------------------------------------------------------------------------|
| proxy/config.go | Added `Macros` field to `Config` struct; enhanced `LoadConfigFromReader` with macro validation, substitution, and error handling for unknown and reserved macros. |
| proxy/config_test.go | Updated existing tests and added new tests for macro replacement, validation, and error handling in config loading. |
| README.md | Simplified configuration example by removing detailed YAML and replacing it with a concise bullet list and a link to full docs. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant ConfigLoader
participant ModelConfig
User->>ConfigLoader: Provide config YAML (with macros)
ConfigLoader->>ConfigLoader: Parse macros and validate names/values
ConfigLoader->>ModelConfig: Substitute macros in fields (cmd, cmdStop, proxy, checkEndpoint)
ConfigLoader->>ConfigLoader: Substitute ${PORT} if present and increment port
ConfigLoader->>ConfigLoader: Check for unreplaced/unknown macros
ConfigLoader-->>User: Return loaded Config or errorPossibly related PRs
|
mostlygeek
changed the title
mostlygeek
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proxy/config.go (1)
192-202: Consider including checkEndpoint in PORT substitution for consistency.Currently, user-defined macros are replaced in
checkEndpoint(line 189), but the special${PORT}macro is not (lines 199-201). This inconsistency could confuse users who might expect${PORT}to work in all fields where macros are supported.Consider adding checkEndpoint to the PORT substitution:
modelConfig.Cmd = strings.ReplaceAll(modelConfig.Cmd, "${PORT}", nextPortStr) modelConfig.CmdStop = strings.ReplaceAll(modelConfig.CmdStop, "${PORT}", nextPortStr) modelConfig.Proxy = strings.ReplaceAll(modelConfig.Proxy, "${PORT}", nextPortStr) + modelConfig.CheckEndpoint = strings.ReplaceAll(modelConfig.CheckEndpoint, "${PORT}", nextPortStr)proxy/config_test.go (1)
372-441: Comprehensive error testing with table-driven approach.The table-driven test structure effectively covers error cases for unknown macros in all supported fields. Good practice!
Consider removing the commented debug line:
assert.Contains(t, err.Error(), "unknown macro '${unknownMacro}' found in model1."+tt.field) - //t.Log(err) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
-
proxy/config.go(4 hunks) -
proxy/config_test.go(5 hunks)
🔇 Additional comments (6)
proxy/config.go (4)
71-73: LGTM!The
Macrosfield is well-placed and properly documented.
183-191: Well-implemented macro substitution logic.The implementation correctly handles macro replacement in all relevant model fields and maintains proper order of operations to support nested macros.
207-225: Excellent unknown macro detection implementation.The validation correctly identifies any unreplaced macros after all substitutions and provides clear error messages with the specific field location.
157-165:⚠️ Potential issueFix off-by-one error in length validation.
The length checks are inconsistent with the comments. The code rejects values at exactly 64 and 1024 characters, while the comments indicate these should be the maximum allowed lengths.
Apply this diff to fix the length validation:
- if len(macroName) >= 64 { + if len(macroName) > 64 { return Config{}, fmt.Errorf("macro name '%s' exceeds maximum length of 63 characters", macroName) } if !macroNameRegex.MatchString(macroName) { return Config{}, fmt.Errorf("macro name '%s' contains invalid characters, must match pattern ^[a-zA-Z0-9_-]+$", macroName) } - if len(macroValue) >= 1024 { + if len(macroValue) > 1024 { return Config{}, fmt.Errorf("macro value for '%s' exceeds maximum length of 1024 characters", macroName) }Also update the error messages to match:
- return Config{}, fmt.Errorf("macro name '%s' exceeds maximum length of 63 characters", macroName) + return Config{}, fmt.Errorf("macro name '%s' exceeds maximum length of 64 characters", macroName)Likely an incorrect or invalid review comment.
proxy/config_test.go (2)
22-83: LGTM! Test properly validates macro functionality.The test correctly includes macros in the configuration and verifies that the macro substitution works as expected.
340-370: Excellent test coverage for complex macro scenarios.This test effectively validates:
- Multiple macro substitutions
- Nested macros (${autoPort} containing ${PORT})
- Multi-line command handling
- Macro substitution in both cmd and cmdStop fields
Great test design!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
60-70: Enhance clarity and example coverage for macros.
- The phrase “very powerful” can be softened (e.g., “highly capable” or “extremely flexible”) to avoid overused intensifiers.
- Since macros are the key feature introduced by this PR, consider adding a minimal
macrossection directly below theconfig.yamlsnippet to illustrate their definition and usage. For example:macros: llamaSrv: "/app/llama-server -hf bartowski/SmolLM2-135M-Instruct-GGUF:Q4_K_M" models: "example-model": cmd: | ${llamaSrv} --port ${PORT}This will help users immediately see how to declare and reference their own macros without having to jump to the wiki.
🧰 Tools
🪛 LanguageTool
[style] ~60-~60: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ..._K_M --port ${PORT} ``` But also very powerful: - ⚡groupsto run multiple models a...(EN_WEAK_ADJECTIVE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~60-~60: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ..._K_M --port ${PORT} ``` But also very powerful: - ⚡ groups to run multiple models a...
(EN_WEAK_ADJECTIVE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests
🔇 Additional comments (2)
README.md (2)
51-51: Correct use of${PORT}placeholder.
This concise snippet demonstrates the intended automatic port substitution via${PORT}in model commands, aligning with the new macro system.
58-58: Consistent${PORT}usage.
The second model example correctly applies the${PORT}placeholder. Good consistency across examples.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref: #141, #148
cmd,cmdStop,proxy, andcheckEndpoint${PORT}can be used in macrosAdd support for defining and using macro in the configuration:
The
cmdwill come out as:/path/to/llama-server/llama-server-293924 --port 5800 --host 127.0.0.1 --cache-type-k q8_0 --cache-type-v q8_0 -m "/path/to/my/mnt/drive/models/llama-8B.gguf"Summary by CodeRabbit
New Features
Documentation
Tests