-
-
Notifications
You must be signed in to change notification settings - Fork 296
feat: Add support for VertexAI compatible service #375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Summary of ChangesHello @aeroxy, 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 significantly extends the system's capabilities by adding comprehensive support for Vertex AI-compatible services. This allows for seamless integration with external providers that adhere to Vertex AI's API patterns but utilize simpler API key-based authentication. The changes span configuration, client management, and API request execution, ensuring that these new services are fully recognized and functional within the existing framework. Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds support for a new VertexAI-compatible service. This includes new configuration options, a new executor, and updates to the watcher and service logic to integrate the new provider. The changes are mostly correct and follow the existing patterns in the codebase. However, I've identified a few issues:
- A significant amount of code in the new
vertex_compat_executor.gois duplicated from other executors, which could be refactored to improve maintainability. - There is a redundant and incorrect block of code in
internal/watcher/watcher.gothat creates duplicateAuthobjects. - A new file
sdk/auth/vertex_compat.goseems to be unused and contains logic that is inconsistent with other parts of the code.
I've provided specific comments and suggestions to address these points.
internal/watcher/watcher.go
Outdated
| // Vertex-compatible API keys -> synthesize auths | ||
| for i := range cfg.VertexCompatAPIKey { | ||
| entry := cfg.VertexCompatAPIKey[i] | ||
| key := strings.TrimSpace(entry.APIKey) | ||
| base := strings.TrimSpace(entry.BaseURL) | ||
| if key == "" || base == "" { | ||
| continue | ||
| } | ||
| proxyURL := strings.TrimSpace(entry.ProxyURL) | ||
| id, token := idGen.next("vertex-compat:apikey", key, base) | ||
| attrs := map[string]string{ | ||
| "source": fmt.Sprintf("config:vertex-compat[%s]", token), | ||
| "api_key": key, | ||
| "base_url": base, | ||
| } | ||
| addConfigHeadersToAttrs(entry.Headers, attrs) | ||
| a := &coreauth.Auth{ | ||
| ID: id, | ||
| Provider: "vertex-compat", | ||
| Label: fmt.Sprintf("vertex-compat-%s", token), | ||
| Status: coreauth.StatusActive, | ||
| ProxyURL: proxyURL, | ||
| Attributes: attrs, | ||
| CreatedAt: now, | ||
| UpdatedAt: now, | ||
| } | ||
| out = append(out, a) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code, which synthesizes auth objects for Vertex-compatible API keys, appears to be redundant and incorrect. There is another loop further down in the same function (lines 1060-1093) that also handles VertexCompatAPIKey. The second loop correctly handles model aliases by computing a models_hash, which this block does not. This duplication will lead to incorrect behavior and duplicate auth entries.
This block should be removed.
| // VertexCompatExecutor is a stateless executor for Vertex AI-compatible APIs | ||
| // that use Vertex-style paths (/publishers/google/models/{model}:action) | ||
| // but authenticate with simple API keys instead of Google Cloud service accounts. | ||
| // | ||
| // This executor supports third-party providers like zenmux.ai that mimic | ||
| // Vertex AI's URL structure while using simpler authentication mechanisms. | ||
| type VertexCompatExecutor struct { | ||
| cfg *config.Config | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new executor contains a significant amount of code that is duplicated from gemini_executor.go and gemini_vertex_executor.go. The Execute, ExecuteStream, and CountTokens methods are nearly identical across these files, with minor differences in URL construction and authentication.
To improve maintainability and reduce redundancy, consider refactoring the common logic into a base executor struct or a set of shared helper functions. This would make the codebase easier to manage and reduce the chance of bugs when updating the execution logic for Gemini-like providers.
sdk/auth/vertex_compat.go
Outdated
| package auth | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/router-for-me/CLIProxyAPI/v6/internal/config" | ||
| coreauth "github.com/router-for-me/CLIProxyAPI/v6/sdk/cliproxy/auth" | ||
| ) | ||
|
|
||
| // LoadVertexCompatCredentials loads Vertex AI-compatible API key credentials from config | ||
| // into the auth manager. Each entry becomes an in-memory auth entry with API key and | ||
| // custom headers stored in attributes. | ||
| func LoadVertexCompatCredentials(ctx context.Context, cfg *config.Config, mgr *coreauth.Manager) error { | ||
| if cfg == nil || mgr == nil { | ||
| return nil | ||
| } | ||
| if len(cfg.VertexCompatAPIKey) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| for i, entry := range cfg.VertexCompatAPIKey { | ||
| apiKey := strings.TrimSpace(entry.APIKey) | ||
| baseURL := strings.TrimSpace(entry.BaseURL) | ||
|
|
||
| if apiKey == "" || baseURL == "" { | ||
| continue | ||
| } | ||
|
|
||
| // Create unique ID from index and base URL | ||
| id := fmt.Sprintf("vertex-compat-%d-%s", i, sanitizeForID(baseURL)) | ||
| label := fmt.Sprintf("Vertex-Compat (%s)", extractDomain(baseURL)) | ||
|
|
||
| // Build attributes map | ||
| attrs := make(map[string]string) | ||
| attrs["api_key"] = apiKey | ||
| attrs["base_url"] = baseURL | ||
|
|
||
| // Add proxy URL if specified | ||
| if entry.ProxyURL != "" { | ||
| attrs["proxy_url"] = strings.TrimSpace(entry.ProxyURL) | ||
| } | ||
|
|
||
| // Copy custom headers to attributes with "header_" prefix | ||
| for k, v := range entry.Headers { | ||
| headerKey := "header_" + strings.ToLower(strings.TrimSpace(k)) | ||
| attrs[headerKey] = strings.TrimSpace(v) | ||
| } | ||
|
|
||
| auth := &coreauth.Auth{ | ||
| ID: id, | ||
| Provider: "vertex-compat", | ||
| Label: label, | ||
| Attributes: attrs, | ||
| Metadata: make(map[string]any), | ||
| } | ||
|
|
||
| if _, err := mgr.Register(ctx, auth); err != nil { | ||
| return fmt.Errorf("failed to register vertex-compat credential %d: %w", i, err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // sanitizeForID creates a safe ID component from a URL. | ||
| func sanitizeForID(url string) string { | ||
| // Remove https:// and http:// | ||
| clean := strings.TrimPrefix(url, "https://") | ||
| clean = strings.TrimPrefix(clean, "http://") | ||
| // Replace non-alphanumeric with dash | ||
| clean = strings.Map(func(r rune) rune { | ||
| if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') { | ||
| return r | ||
| } | ||
| return '-' | ||
| }, clean) | ||
| // Limit length | ||
| if len(clean) > 30 { | ||
| clean = clean[:30] | ||
| } | ||
| return strings.Trim(clean, "-") | ||
| } | ||
|
|
||
| // extractDomain extracts the domain from a URL for display purposes. | ||
| func extractDomain(url string) string { | ||
| clean := strings.TrimPrefix(url, "https://") | ||
| clean = strings.TrimPrefix(clean, "http://") | ||
| if idx := strings.Index(clean, "/"); idx > 0 { | ||
| clean = clean[:idx] | ||
| } | ||
| return clean | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new file appears to be unused in the current pull request. The function LoadVertexCompatCredentials duplicates logic for creating Auth objects from the configuration, which is also handled in internal/watcher/watcher.go. The logic here is also inconsistent with the watcher's implementation (e.g., ID generation, handling of model aliases).
If this file is not intended to be used, it should be removed to avoid confusion and dead code. If it is intended for future use, it should be reconciled with the logic in watcher.go to ensure consistency.
luispater
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer to internal/runtime/executor/claude_executor.go to add apikey support within internal/runtime/executor/gemini_vertex_executor.go, instead of using a separate executor to add this support.
Thank you.
Got it. :) Will do. |
d313452 to
0fdce3e
Compare
No description provided.