-
Notifications
You must be signed in to change notification settings - Fork 2
feature/evaluate invoke FIRE 1110 #468
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?
Conversation
WalkthroughBumps project version (0.15.0 → 0.16.0), changes default API base URL to https://api.qualifire.ai/, updates client evaluation endpoints to /api/v1/evaluation/..., and adds tracing URL constants and a new get_tracing_url() util used by tracer initialization. Changes
Sequence Diagram(s)(omitted — changes are configuration and minor endpoint adjustments without new multi-component control flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@qualifire/consts.py`:
- Around line 3-5: Restore the proxy default by changing _DEFAULT_BASE_URL back
to "https://proxy.qualifire.ai/" (so get_base_url() falls back to the proxy),
keep _DEFAULT_TRACING_URL as-is, and ensure environment variable names used for
resolution are QUALIFIRE_API_KEY for the API key and QUALIFIRE_BASE_URL for
overriding the base URL; update any references or resolution logic that
currently use QUALIFIRE_TRACING_URL or the new hardcoded
"https://api.qualifire.ai/" to use these constants (_DEFAULT_BASE_URL,
QUALIFIRE_BASE_URL env) so behavior remains backward-compatible unless
deliberately making a breaking change.
🧹 Nitpick comments (1)
qualifire/utils.py (1)
26-30: Normalize tracing URL to avoid double slashes.If
QUALIFIRE_TRACING_URLincludes a trailing/, the constructed endpoint becomes//api/telemetry. Consider stripping trailing slashes once in the helper.♻️ Suggested update
def get_tracing_url() -> str: - tracing_url = os.getenv(QUALIFIRE_TRACING_URL_ENV_VAR) - if not tracing_url: - return _DEFAULT_TRACING_URL - return tracing_url + tracing_url = os.getenv(QUALIFIRE_TRACING_URL_ENV_VAR) or _DEFAULT_TRACING_URL + return tracing_url.rstrip("/")
| QUALIFIRE_TRACING_URL_ENV_VAR = "QUALIFIRE_TRACING_URL" | ||
| _DEFAULT_BASE_URL = "https://api.qualifire.ai/" | ||
| _DEFAULT_TRACING_URL = "https://tracing.qualifire.ai" |
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.
Default base URL should remain the proxy endpoint.
Changing _DEFAULT_BASE_URL to https://api.qualifire.ai/ alters the fallback behavior for get_base_url() and conflicts with the expected proxy default. If the proxy is still the supported default, keep the prior value or explicitly treat this as a breaking change.
✅ Suggested fix
-_DEFAULT_BASE_URL = "https://api.qualifire.ai/"
+_DEFAULT_BASE_URL = "https://proxy.qualifire.ai/"Based on learnings use environment variables QUALIFIRE_API_KEY for API key and QUALIFIRE_BASE_URL for base URL resolution (defaults to https://proxy.qualifire.ai/).
📝 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.
| QUALIFIRE_TRACING_URL_ENV_VAR = "QUALIFIRE_TRACING_URL" | |
| _DEFAULT_BASE_URL = "https://api.qualifire.ai/" | |
| _DEFAULT_TRACING_URL = "https://tracing.qualifire.ai" | |
| QUALIFIRE_TRACING_URL_ENV_VAR = "QUALIFIRE_TRACING_URL" | |
| _DEFAULT_BASE_URL = "https://proxy.qualifire.ai/" | |
| _DEFAULT_TRACING_URL = "https://tracing.qualifire.ai" |
🤖 Prompt for AI Agents
In `@qualifire/consts.py` around lines 3 - 5, Restore the proxy default by
changing _DEFAULT_BASE_URL back to "https://proxy.qualifire.ai/" (so
get_base_url() falls back to the proxy), keep _DEFAULT_TRACING_URL as-is, and
ensure environment variable names used for resolution are QUALIFIRE_API_KEY for
the API key and QUALIFIRE_BASE_URL for overriding the base URL; update any
references or resolution logic that currently use QUALIFIRE_TRACING_URL or the
new hardcoded "https://api.qualifire.ai/" to use these constants
(_DEFAULT_BASE_URL, QUALIFIRE_BASE_URL env) so behavior remains
backward-compatible unless deliberately making a breaking change.
Description
Related Issue
Type of Change
Checklist
CODE_OF_CONDUCT.mddocument.CONTRIBUTING.mdguide.make codestyle.Summary by CodeRabbit
Chores
New Features
✏️ Tip: You can customize this high-level summary in your review settings.