-
Notifications
You must be signed in to change notification settings - Fork 0
fix(copilot): respect COPILOT_API_BASE_URL and fix follow-up failing check regressions #664
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
base: main
Are you sure you want to change the base?
Changes from all commits
aacf120
d6e8438
e8d4217
5526657
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -27,7 +27,7 @@ | |||||||||||||||
| resolve_external_process_provider_credentials, | ||||||||||||||||
| has_usable_secret, | ||||||||||||||||
| ) | ||||||||||||||||
| from hermes_cli.config import get_compatible_custom_providers, load_config | ||||||||||||||||
| from hermes_cli.config import get_compatible_custom_providers, get_env_value, load_config | ||||||||||||||||
| from hermes_constants import OPENROUTER_BASE_URL | ||||||||||||||||
| from utils import base_url_host_matches, base_url_hostname | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -220,7 +220,20 @@ def _resolve_runtime_from_pool_entry( | |||||||||||||||
| api_mode = "chat_completions" | ||||||||||||||||
| elif provider == "copilot": | ||||||||||||||||
| api_mode = _copilot_runtime_api_mode(model_cfg, getattr(entry, "runtime_api_key", "")) | ||||||||||||||||
| base_url = base_url or PROVIDER_REGISTRY["copilot"].inference_base_url | ||||||||||||||||
| pconfig = PROVIDER_REGISTRY["copilot"] | ||||||||||||||||
| pool_url_is_default = base_url.rstrip("/") == pconfig.inference_base_url.rstrip("/") | ||||||||||||||||
| if not base_url or pool_url_is_default: | ||||||||||||||||
| cfg_provider = str(model_cfg.get("provider") or "").strip().lower() | ||||||||||||||||
| cfg_base_url = "" | ||||||||||||||||
| if cfg_provider == "copilot": | ||||||||||||||||
| cfg_base_url = str(model_cfg.get("base_url") or "").strip().rstrip("/") | ||||||||||||||||
| env_url = "" | ||||||||||||||||
| if pconfig.base_url_env_var: | ||||||||||||||||
| env_url = ( | ||||||||||||||||
| get_env_value(pconfig.base_url_env_var) | ||||||||||||||||
| or os.getenv(pconfig.base_url_env_var, "") | ||||||||||||||||
| ).strip().rstrip("/") | ||||||||||||||||
|
Comment on lines
+231
to
+235
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gemini-code-assist, is this suggestion still valid? If so, what are the steps and line numbers to make this fix? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the suggestion remains valid.
Suggested change
Comment on lines
+231
to
+235
|
||||||||||||||||
| base_url = cfg_base_url or env_url or base_url or pconfig.inference_base_url | ||||||||||||||||
| elif provider == "azure-foundry": | ||||||||||||||||
| # Azure Foundry: read api_mode and base_url from config | ||||||||||||||||
| cfg_provider = str(model_cfg.get("provider") or "").strip().lower() | ||||||||||||||||
|
|
||||||||||||||||
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.
During pool seeding, we should prefer the
.envfile overos.environto prevent stale inherited environment variables from being persisted intoauth.json(consistent with the fix for NousResearch#18254). Additionally,get_env_valuealready falls back to.envif not inos.environ, making theor os.getenv(...)redundant.We can use
load_env()(which is already imported) to check the.envfile first.