Configuration base url update#110
Conversation
…he code Signed-off-by: Stephanie <yangcao@redhat.com>
…on into base-url-update
WalkthroughConfigurable API versioning (default "v1") was introduced and propagated: configs, CLI args, client constructors, endpoint path construction, and tests were updated to use dynamic f"/{version}/..." paths instead of hard-coded "/v1/...". Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py (1)
118-123: Critical: Inconsistent endpoint path for streaming query.The non-streaming endpoint was updated to
/query(line 60) to avoid duplication with/v1in the base URL, but the streaming endpoint still uses/v1/streaming_query. This inconsistency will cause the streaming endpoint to become/v1/v1/streaming_querywhen combined with the updated base URL that now includes/v1.Apply this diff to fix the inconsistency:
with self.client.stream( "POST", - "/v1/streaming_query", + "/streaming_query", json=api_input, timeout=timeout, ) as response:src/lightspeed_evaluation/core/constants.py (1)
1-7: Fix malformedDEFAULT_API_BASEURL (missing slash)
DEFAULT_API_BASEis currently"http://localhost:8080v1", which is missing the/between the port and version and will produce an invalid base URL when used.Recommend correcting to:
-DEFAULT_API_BASE = "http://localhost:8080v1" +DEFAULT_API_BASE = "http://localhost:8080/v1"This matches the README, config examples, and the test fixtures that use
http://localhost:8080/v1.src/lightspeed_evaluation/core/api/client.py (1)
124-131: Remove leading slash from paths to match versioned base_url strategyThe review comment's concern is confirmed valid. According to RFC 3986 (which httpx implements), a request path with a leading slash like
"/query"is treated as an absolute-path reference that replaces the base URL's path component, not appends to it:
base_url="http://localhost:8080/v1"+client.post("/query", ...)→http://localhost:8080/query❌base_url="http://localhost:8080/v1"+client.post("query", ...)→http://localhost:8080/v1/query✓The current code drops the
/v1prefix from standard queries due to the leading slash. Additionally,_streaming_querystill hardcodes"/v1/streaming_query", creating an inconsistent approach:
- Use relative paths without leading slash (
"query"and"streaming_query") so they merge correctly with the versionedapi_base(e.g.,http://localhost:8080/v1).- Or revert to host-only
api_base(e.g.,http://localhost:8080) and restore versioned paths ("/v1/query","/v1/streaming_query").Apply one strategy consistently across both endpoints.
🧹 Nitpick comments (1)
tests/unit/core/api/test_client.py (1)
10-19: Tests now use versionedapi_base; consider asserting on request pathsUpdating
api_basein these tests tohttp://localhost:8080/v1keeps them consistent with the new default configuration and README examples, which is good.If you want stronger protection against regressions around URL construction, you might optionally extend one of the tests to assert the first argument passed to
mock_client.post/mock_client.stream(the path), so the intended behavior with a versionedapi_baseis locked in at the test level.Also applies to: 29-36, 190-198
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
README-generate-answers.md(1 hunks)README.md(2 hunks)config/system.yaml(1 hunks)lsc_agent_eval/README.md(3 hunks)lsc_agent_eval/src/lsc_agent_eval/agent_eval.py(1 hunks)lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py(1 hunks)lsc_agent_eval/tests/core/agent_goal_eval/test_agent_goal_eval.py(2 hunks)lsc_agent_eval/tests/core/utils/test_api_client.py(15 hunks)lsc_agent_eval/tests/test_agent_eval.py(1 hunks)pyproject.toml(1 hunks)src/generate_answers/eval_config.yaml(1 hunks)src/generate_answers/ls_response.py(1 hunks)src/lightspeed_evaluation/core/api/client.py(1 hunks)src/lightspeed_evaluation/core/constants.py(1 hunks)src/lightspeed_evaluation/core/llm/custom.py(2 hunks)tests/unit/core/api/test_client.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
config/system.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Add new metrics metadata to
config/system.yamlin the metrics_metadata section
Files:
config/system.yaml
config/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Add sample evaluation data YAML file when adding new features
Files:
config/system.yaml
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use type hints for all public functions and methods
Use Google-style docstrings for all public APIs
Use custom exceptions fromcore.system.exceptionsfor error handling
Use structured logging with appropriate levels
Files:
src/generate_answers/ls_response.pysrc/lightspeed_evaluation/core/constants.pysrc/lightspeed_evaluation/core/api/client.pysrc/lightspeed_evaluation/core/llm/custom.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest with pytest-mock (mocker fixture), not unittest.mock, for all mocking
Test files should use naming conventiontest_*.pyfor files,test_*for functions, andTest*for classes
Files:
tests/unit/core/api/test_client.py
🧠 Learnings (13)
📚 Learning: 2025-09-08T11:09:18.416Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 47
File: config/system.yaml:16-20
Timestamp: 2025-09-08T11:09:18.416Z
Learning: The maintainer asamal4 prefers sample configurations in config/system.yaml to show realistic usage patterns (e.g., API enabled: true) rather than conservative defaults, as teams are expected to customize the configuration for their specific environments.
Applied to files:
config/system.yaml
📚 Learning: 2025-09-09T14:58:10.630Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 47
File: src/lightspeed_evaluation/pipeline/evaluation/amender.py:32-41
Timestamp: 2025-09-09T14:58:10.630Z
Learning: In the lightspeed-evaluation framework, when API is enabled, every turn should make a fresh API call regardless of whether the turn already has response or tool_calls data. This ensures consistency and fresh responses for each evaluation run.
Applied to files:
README.md
📚 Learning: 2025-09-09T08:08:40.654Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 47
File: src/lightspeed_evaluation/core/api/__init__.py:3-3
Timestamp: 2025-09-09T08:08:40.654Z
Learning: API_KEY and OPENAI_API_KEY are distinct environment variables with different purposes in the lightspeed-evaluation codebase - they should not be treated as interchangeable fallbacks.
Applied to files:
README.md
📚 Learning: 2025-07-16T12:07:29.169Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 19
File: lsc_agent_eval/tests/core/agent_goal_eval/test_script_runner.py:0-0
Timestamp: 2025-07-16T12:07:29.169Z
Learning: In the lsc_agent_eval package, the ScriptRunner class was modified to use absolute paths internally rather than documenting path normalization behavior, providing more predictable and consistent path handling.
Applied to files:
lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.pylsc_agent_eval/src/lsc_agent_eval/agent_eval.pylsc_agent_eval/tests/test_agent_eval.pylsc_agent_eval/tests/core/agent_goal_eval/test_agent_goal_eval.pylsc_agent_eval/README.mdlsc_agent_eval/tests/core/utils/test_api_client.py
📚 Learning: 2025-11-24T16:59:21.420Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:59:21.420Z
Learning: Do not add features to the legacy `lsc_agent_eval/` directory; use `src/lightspeed_evaluation/` instead
Applied to files:
lsc_agent_eval/src/lsc_agent_eval/agent_eval.pysrc/generate_answers/eval_config.yamllsc_agent_eval/tests/core/agent_goal_eval/test_agent_goal_eval.pylsc_agent_eval/README.md
📚 Learning: 2025-07-29T05:15:39.782Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 22
File: lsc_agent_eval/src/lsc_agent_eval/core/agent_goal_eval/evaluator.py:87-100
Timestamp: 2025-07-29T05:15:39.782Z
Learning: In the lsc_agent_eval framework, the substring evaluation logic in the `_evaluate_substring` method requires ALL expected keywords to be present in the agent response (logical AND), not just any keyword (logical OR). This is a stricter evaluation condition that was intentionally changed and may be subject to future modifications.
Applied to files:
lsc_agent_eval/tests/test_agent_eval.pylsc_agent_eval/tests/core/agent_goal_eval/test_agent_goal_eval.py
📚 Learning: 2025-09-19T01:26:54.625Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 55
File: src/lightspeed_evaluation/core/metrics/manager.py:50-81
Timestamp: 2025-09-19T01:26:54.625Z
Learning: In the lightspeed-evaluation codebase, prefer clean code over defensive programming for controlled configuration files like system.yaml. The maintainer asamal4 prefers not to add error handling for user configuration mistakes that are unlikely to occur in controlled internal settings, to avoid code complexity.
Applied to files:
src/generate_answers/eval_config.yaml
📚 Learning: 2025-09-01T07:25:15.021Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 35
File: pyproject.toml:65-65
Timestamp: 2025-09-01T07:25:15.021Z
Learning: The generate_answers console script entry point in pyproject.toml is intentionally not included in the wheel packages list because it's temporary and won't be packaged in future versions of the lightspeed-evaluation project.
Applied to files:
src/generate_answers/eval_config.yaml
📚 Learning: 2025-11-24T16:59:21.420Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:59:21.420Z
Learning: All new evaluation features should be added to `src/lightspeed_evaluation/` core framework
Applied to files:
src/generate_answers/eval_config.yaml
📚 Learning: 2025-11-24T16:59:09.244Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:59:09.244Z
Learning: Review AGENTS.md - this file contains agent-related guidelines and documentation
Applied to files:
lsc_agent_eval/README.md
📚 Learning: 2025-08-22T09:16:29.070Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 26
File: lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py:124-151
Timestamp: 2025-08-22T09:16:29.070Z
Learning: In lsc_agent_eval project, the maintainer (asamal4) prefers reactive error handling - adding support for additional error response fields only when they occur in practice, rather than preemptively handling all possible error formats.
Applied to files:
lsc_agent_eval/README.md
📚 Learning: 2025-07-16T09:42:00.691Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 19
File: lsc_agent_eval/data/script/eval3/setup.sh:1-3
Timestamp: 2025-07-16T09:42:00.691Z
Learning: Scripts in the lsc_agent_eval/data directory are meant to be simple examples/samples for teams to customize according to their needs, not production-ready code.
Applied to files:
lsc_agent_eval/README.md
📚 Learning: 2025-07-16T13:20:45.006Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 19
File: lsc_agent_eval/tests/core/agent_goal_eval/test_evaluator.py:0-0
Timestamp: 2025-07-16T13:20:45.006Z
Learning: In the lsc_agent_eval package, evaluation results use distinct values: "FAIL" means the evaluation ran successfully but the result was negative, while "ERROR" means there was an issue executing the evaluation itself (e.g., setup script failed, API connection failed).
Applied to files:
lsc_agent_eval/README.md
🧬 Code graph analysis (2)
lsc_agent_eval/tests/core/agent_goal_eval/test_agent_goal_eval.py (1)
lsc_agent_eval/tests/core/agent_goal_eval/test_evaluator.py (1)
mock_agent_client(24-40)
src/lightspeed_evaluation/core/llm/custom.py (1)
src/lightspeed_evaluation/core/system/exceptions.py (1)
LLMError(24-25)
🪛 GitHub Actions: Pyright
src/lightspeed_evaluation/core/llm/custom.py
[error] 78-78: Except clause is unreachable because exception is already handled
🪛 GitHub Actions: Python linter
src/lightspeed_evaluation/core/llm/custom.py
[error] 78-78: Pylint W0705: Catching previously caught exception type Exception (duplicate-except)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: tests (3.12)
- GitHub Check: tests (3.11)
- GitHub Check: tests (3.13)
- GitHub Check: mypy
🔇 Additional comments (12)
README-generate-answers.md (1)
16-16: LGTM!The documentation correctly reflects the updated base URL pattern that now includes the
/v1version prefix.lsc_agent_eval/src/lsc_agent_eval/agent_eval.py (1)
34-34: LGTM!The default endpoint correctly includes the
/v1version prefix, aligning with the PR's objective to move versioning into the base URL configuration.src/generate_answers/eval_config.yaml (1)
1-1: LGTM!The configuration correctly includes the
/v1version prefix in the base URL, consistent with the PR's objective.config/system.yaml (1)
33-33: LGTM!The API base URL correctly includes the
/v1version prefix, aligning with the centralized versioning approach described in the PR.lsc_agent_eval/README.md (1)
203-203: LGTM!The documentation examples correctly demonstrate the updated endpoint pattern with
/v1included in the base URL.Also applies to: 221-221, 239-239
pyproject.toml (1)
12-12: I'll help you verify the ragas version pin change. Let me gather information about the latest ragas version and any known compatibility issues.
<function_calls>
ragas latest version 2025
</function_calls>Alternatively, let me also check the repository context to understand the PR better:
<function_calls>
#!/bin/bashCheck the current pyproject.toml to see context around the ragas dependency
head -20 pyproject.toml
</function_calls>
<function_calls>
#!/bin/bashCheck git log to see recent changes to pyproject.toml
git log --oneline -10 -- pyproject.toml 2>/dev/null || echo "Git history not available"
</function_calls>Please wait for the results from these queries. They will help me determine:
- The current latest version of ragas
- Any known compatibility issues with version 0.3.8
- Whether this version pin change aligns with the PR's stated goals
- Context from the repository about why this change was made
lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py (1)
60-60: I'll help you verify the consistency of the API client update. Let me start by examining the api_client.py file and then verify all instantiations of AgentHttpClient.
<function_calls>
#!/bin/bashFirst, let's find and examine the api_client.py file
fd -name "api_client.py" --type f
</function_calls>Let me get more context around line 60 and understand the full picture of the changes:
<function_calls>
#!/bin/bashRead the api_client.py file with line numbers
wc -l "lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py"
head -80 "lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py"
</function_calls>Now let me verify all AgentHttpClient instantiations as suggested in the review comment:
<function_calls>
#!/bin/bashVerify all AgentHttpClient instantiations include /v1 in the endpoint
rg -nP -C3 '\bAgentHttpClient\s*(' --type=py
</function_calls>Let me also search for where the base URL is defined and how it's being used:
<function_calls>
#!/bin/bashSearch for base URL definitions and /v1 references
rg -nP "base_url|/v1|/query" --type=py -C2
</function_calls>Let me check for any tests related to the api_client:
<function_calls>
#!/bin/bashFind test files related to api_client
fd -name "testapi*" --type f
fd -name "apitest*" --type fAlso search for test files in common locations
find . -type f -name "test_.py" | grep -i api
find . -type f -name "_test.py" | grep -i api
</function_calls>src/generate_answers/ls_response.py (1)
68-68: Verify all LSClient instantiations include/v1in thels_urlparameter.The endpoint has been correctly updated to
/queryto work with base URLs that now include the/v1prefix. Ensure that all instantiations ofLSClientpass als_urlparameter that includes the version (e.g.,http://localhost:8080/v1).README.md (1)
56-61: Doc default base URL now correctly includes/v1The documented default
http://localhost:8080/v1matches the updated system config example and the PR goal of making the version part ofapi_base. Looks good.lsc_agent_eval/tests/test_agent_eval.py (1)
24-33: Test expectation updated to match versioned default endpointThe minimal-args test now expects
agent_endpoint == "http://localhost:8080/v1", which is consistent with the new default base URL pattern used elsewhere in the repo. No issues here.lsc_agent_eval/tests/core/agent_goal_eval/test_agent_goal_eval.py (1)
20-33: Agent endpoint tests align with versioned base URLThe fixture and expectation now consistently use
http://localhost:8080/v1foragent_endpoint, which matches the updated defaults and other tests. Wiring intoAgentHttpClientstill looks correct.Also applies to: 102-113
src/lightspeed_evaluation/core/llm/custom.py (1)
44-55: I've encountered a repository access limitation that prevents me from directly verifying all aspects of the review comment. However, I have been able to verify some key points:What I verified:
- The
litellm.completion()withresponse_format={"type":"json_object"}does enable JSON mode, which forces the assistant message to be valid JSON.- Provider and model-level support varies significantly across OpenAI, Azure, Google, Anthropic, Bedrock, and others.
litellmcan throwUnsupportedParamsErrorif a specific model/provider doesn't support the parameter.- Developers must still instruct the model to output JSON in the prompt itself;
response_formatalone doesn't guarantee usable JSON without prompt guidance.What I cannot verify without repository access:
- Whether the duplicate
except Exception as e:blocks actually exist at lines 75-79- Whether the prompts have been updated to explicitly request JSON output
- All call sites of
BaseCustomLLM.call()and whether they expect free-form text or JSON- Whether this change was intentional or accidental
Given these limitations, I need to mark this for manual verification:
**Verify duplicate `except` block and validate `response_format={"type":"json_object"}` global behavior** Repository access is currently unavailable, preventing full verification of: 1. **Duplicate `except Exception` block** — cannot confirm lines 75-79 contain the unreachable handler flagged by pylint W0705. 2. **Global JSON-object response format** — the concern is valid in principle: forcing `response_format={"type":"json_object"}` across all `litellm.completion()` calls requires: - All target models/providers support JSON mode (varies by provider; unsupported combinations raise `UnsupportedParamsError`) - Prompts explicitly instruct the model to output JSON (the parameter alone does not guarantee parseable JSON) - All callers of `BaseCustomLLM.call()` expect JSON or can handle it as an opaque string Before merging, confirm these conditions are met and consider making JSON mode configurable rather than mandatory. Manual review of the file is recommended to validate both issues.
Signed-off-by: Stephanie <yangcao@redhat.com>
yangcao77
force-pushed
the
base-url-update
branch
from
d92ede8 to
f3b2509
Compare
VladimirKadlec
left a comment
VladimirKadlec
left a comment
There was a problem hiding this comment.
I'm OK with the change. Please add the missing trailing slashes.
config/system.yaml
Outdated
| api: | ||
| enabled: true # Enable API calls instead of using pre-filled data | ||
| api_base: http://localhost:8080 # Base API URL | ||
| api_base: http://localhost:8080/v1 # Base API URL |
There was a problem hiding this comment.
Please add trailing slash (on all occurrences)
| api_base: http://localhost:8080/v1 # Base API URL | |
| api_base: http://localhost:8080/v1/ # Base API URL |
| """Common constants for evaluation framework.""" | ||
|
|
||
| DEFAULT_API_BASE = "http://localhost:8080" | ||
| DEFAULT_API_BASE = "http://localhost:8080v1" |
There was a problem hiding this comment.
Missing slash before and after v1
| DEFAULT_API_BASE = "http://localhost:8080v1" | |
| DEFAULT_API_BASE = "http://localhost:8080/v1/" |
Signed-off-by: Stephanie <yangcao@redhat.com>
Signed-off-by: Stephanie <yangcao@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lsc_agent_eval/tests/core/utils/test_api_client.py (2)
97-97: Critical: Test expects leading slash, but implementation should use relative pathThe test asserts
mock_client.postwas called with"/query"(leading slash). However, as noted in the implementation review, using a leading slash withbase_url="http://localhost:8080/v1/"causes incorrect URL resolution (http://localhost:8080/queryinstead ofhttp://localhost:8080/v1/query).Update the test expectation to match the corrected implementation:
- "/query", + "query",Also consider adding an assertion that verifies the final resolved URL, not just the path argument, to catch such issues in the future.
253-253: Critical: Test expects leading slash, but implementation should use relative pathSame issue as line 97. The test expects
"/streaming_query"with a leading slash, which causes incorrect URL resolution when combined with the trailing-slash base URL.Update the test expectation:
- "/streaming_query", + "streaming_query",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py(2 hunks)lsc_agent_eval/tests/core/utils/test_api_client.py(16 hunks)src/lightspeed_evaluation/core/api/client.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lightspeed_evaluation/core/api/client.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-16T12:07:29.169Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 19
File: lsc_agent_eval/tests/core/agent_goal_eval/test_script_runner.py:0-0
Timestamp: 2025-07-16T12:07:29.169Z
Learning: In the lsc_agent_eval package, the ScriptRunner class was modified to use absolute paths internally rather than documenting path normalization behavior, providing more predictable and consistent path handling.
Applied to files:
lsc_agent_eval/tests/core/utils/test_api_client.pylsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py
🧬 Code graph analysis (1)
lsc_agent_eval/tests/core/utils/test_api_client.py (1)
lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py (1)
AgentHttpClient(16-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pylinter
- GitHub Check: pydocstyle
- GitHub Check: mypy
- GitHub Check: tests (3.11)
- GitHub Check: tests (3.12)
- GitHub Check: tests (3.13)
🔇 Additional comments (1)
lsc_agent_eval/tests/core/utils/test_api_client.py (1)
16-24: Good: Base URL now includes trailing slashThe base URL correctly includes the trailing slash (
/v1/), which is necessary for proper relative path resolution. However, the request paths in other tests still use leading slashes (see comments on lines 97 and 253), which will cause URL resolution issues.
| with self.client.stream( | ||
| "POST", | ||
| "/v1/streaming_query", | ||
| "/streaming_query", |
There was a problem hiding this comment.
Critical: Leading slash in path drops base URL path component
Same issue as the non-streaming endpoint. With base_url="http://localhost:8080/v1/" and client.stream("POST", "/streaming_query", ...), the resolved URL becomes http://localhost:8080/streaming_query (the /v1/ is dropped).
Remove the leading slash:
- "/streaming_query",
+ "streaming_query",This ensures the URL resolves correctly to http://localhost:8080/v1/streaming_query.
📝 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.
| "/streaming_query", | |
| "streaming_query", |
🤖 Prompt for AI Agents
In lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py around line 120,
the path string for the streaming endpoint includes a leading slash
("/streaming_query") which causes the base_url path component to be dropped when
resolving (e.g. "http://localhost:8080/v1/" + "/streaming_query" ->
"http://localhost:8080/streaming_query"); remove the leading slash (use
"streaming_query") or normalize path concatenation (strip a leading slash from
provided paths or use urljoin-safe concatenation) so the final URL resolves to
"http://localhost:8080/v1/streaming_query".
asamal4
left a comment
asamal4
left a comment
There was a problem hiding this comment.
@yangcao77 I would prefer to have another property in system.yaml for the version and necessary code to form the endpoint path for API request. This will also avoid any concern about absolute endpoint path.
cc: @VladimirKadlec
| api: | ||
| enabled: true # Enable API calls instead of using pre-filled data | ||
| api_base: http://localhost:8080 # Base API URL | ||
| api_base: http://localhost:8080/v1/ # Base API URL | ||
| endpoint_type: streaming # Use "streaming" or "query" endpoint | ||
| timeout: 300 # API request timeout in seconds |
There was a problem hiding this comment.
| api: | |
| enabled: true # Enable API calls instead of using pre-filled data | |
| api_base: http://localhost:8080 # Base API URL | |
| api_base: http://localhost:8080/v1/ # Base API URL | |
| endpoint_type: streaming # Use "streaming" or "query" endpoint | |
| timeout: 300 # API request timeout in seconds | |
| api: | |
| enabled: true # Enable API calls instead of using pre-filled data | |
| api_base: http://localhost:8080 # Base API URL | |
| endpoint_type: streaming # Use "streaming" or "query" endpoint | |
| version: v1 # Endpoint version | |
| timeout: 300 # API request timeout in seconds |
Signed-off-by: Stephanie <yangcao@redhat.com>
Signed-off-by: Stephanie <yangcao@redhat.com>
Signed-off-by: Stephanie <yangcao@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py (1)
19-31: Versioned paths are wired correctly; ensure endpoint is truly “host-only”The new
AgentHttpClientsignature (endpoint,version,token_file) and the use of"/{self.version}/query"and"/{self.version}/streaming_query"align cleanly with the CLI and tests: withendpoint="http://localhost:8080"andversion="v1", requests go to/v1/...as intended.One nuance to keep in mind: because the request paths start with a leading
/, any path segment included inendpoint(e.g.,http://host/proxy) will be ignored by httpx’s URL resolution and requests will target/{version}/...from the host root. Given the help text now describesendpointas a base URL without version, this is probably acceptable, but it’s worth ensuring we don’t expect to support path-prefix endpoints here.Also applies to: 67-72, 127-132
🧹 Nitpick comments (4)
src/lightspeed_evaluation/core/api/client.py (1)
25-35: Version wiring looks good; consider adding version to cache key
- Storing
config.versiononself.versionand using it in"/{self.version}/query"and"/{self.version}/streaming_query"cleanly enables configurable versioning and matches the new config model. 👍- One follow‑up:
_get_cache_keycurrently hashes only request fields (query/provider/model/no_tools/system_prompt/attachments). If a user flipsAPIConfig.version(e.g., v1 → v2) while reusing the samecache_dir, they can get cached responses from the wrong API version. Consider includingself.versionin the cache key construction so caches are version‑scoped.Also applies to: 125-133, 174-183, 248-263
lsc_agent_eval/src/lsc_agent_eval/agent_eval.py (1)
31-43: CLI version flag is consistent with backend; default duplication is acceptable but could be centralizedThe new
--agent_api_versionargument and the updated--agent_endpointhelp text correctly reflect the “base URL without version + separate version” model and match howAgentGoalEval/AgentHttpClientnow behave.If you want to avoid drifting defaults long‑term, you might consider centralizing the
"v1"default (e.g., via a shared constant), but that’s optional given the package boundary.tests/unit/core/api/test_client.py (1)
10-21: Tests correctly adoptversion; consider adding coverage for non‑default versionsUsing
version="v1"in theapi_configfixture and in the unsupported-endpoint and streaming tests keeps these tests aligned with the newAPIConfigshape andAPIClientexpectations.To exercise the new functionality, consider adding small tests that:
- construct
APIConfigwithversion="v2"(for both query and streaming), and- assert that
mock_client.post/mock_client.streamare called with"/v2/query"/"/v2/streaming_query".That would lock in the versioned-path behavior this PR is introducing.
Also applies to: 40-52, 206-215
lsc_agent_eval/tests/core/utils/test_api_client.py (1)
16-25: AgentHttpClient tests align with versioned API; add one non‑default version test for robustnessThe updates here—passing
version="v1"intoAgentHttpClient, assertingclient.version, and expecting"/v1/query"/"/v1/streaming_query"—all correctly reflect the new client contract and help catch regressions.To fully exercise the new version parameter, consider adding a focused test that constructs
AgentHttpClient(endpoint, version="v2")and asserts that:
mock_client.postis called with"/v2/query", andmock_client.streamis called with"/v2/streaming_query".That would make the behavior for alternate versions explicit and future‑proof.
Also applies to: 34-40, 52-56, 62-63, 85-107, 124-136, 152-199, 206-207, 217-217, 245-262, 287-312, 335-343
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
README.md(1 hunks)config/system.yaml(1 hunks)lsc_agent_eval/src/lsc_agent_eval/agent_eval.py(1 hunks)lsc_agent_eval/src/lsc_agent_eval/core/agent_goal_eval/agent_goal_eval.py(1 hunks)lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py(3 hunks)lsc_agent_eval/tests/core/agent_goal_eval/test_agent_goal_eval.py(2 hunks)lsc_agent_eval/tests/core/utils/test_api_client.py(14 hunks)lsc_agent_eval/tests/test_agent_eval.py(3 hunks)src/lightspeed_evaluation/core/api/client.py(3 hunks)src/lightspeed_evaluation/core/constants.py(1 hunks)src/lightspeed_evaluation/core/models/system.py(2 hunks)tests/unit/core/api/test_client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lsc_agent_eval/tests/core/agent_goal_eval/test_agent_goal_eval.py
- README.md
- config/system.yaml
- lsc_agent_eval/tests/test_agent_eval.py
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use type hints for all public functions and methods
Use Google-style docstrings for all public APIs
Use custom exceptions fromcore.system.exceptionsfor error handling
Use structured logging with appropriate levels
Files:
src/lightspeed_evaluation/core/models/system.pysrc/lightspeed_evaluation/core/constants.pysrc/lightspeed_evaluation/core/api/client.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest with pytest-mock (mocker fixture), not unittest.mock, for all mocking
Test files should use naming conventiontest_*.pyfor files,test_*for functions, andTest*for classes
Files:
tests/unit/core/api/test_client.py
🧠 Learnings (2)
📚 Learning: 2025-07-16T12:07:29.169Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 19
File: lsc_agent_eval/tests/core/agent_goal_eval/test_script_runner.py:0-0
Timestamp: 2025-07-16T12:07:29.169Z
Learning: In the lsc_agent_eval package, the ScriptRunner class was modified to use absolute paths internally rather than documenting path normalization behavior, providing more predictable and consistent path handling.
Applied to files:
lsc_agent_eval/tests/core/utils/test_api_client.pylsc_agent_eval/src/lsc_agent_eval/agent_eval.pylsc_agent_eval/src/lsc_agent_eval/core/agent_goal_eval/agent_goal_eval.py
📚 Learning: 2025-07-29T05:15:39.782Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 22
File: lsc_agent_eval/src/lsc_agent_eval/core/agent_goal_eval/evaluator.py:87-100
Timestamp: 2025-07-29T05:15:39.782Z
Learning: In the lsc_agent_eval framework, the substring evaluation logic in the `_evaluate_substring` method requires ALL expected keywords to be present in the agent response (logical AND), not just any keyword (logical OR). This is a stricter evaluation condition that was intentionally changed and may be subject to future modifications.
Applied to files:
lsc_agent_eval/src/lsc_agent_eval/core/agent_goal_eval/agent_goal_eval.py
🧬 Code graph analysis (1)
lsc_agent_eval/tests/core/utils/test_api_client.py (1)
lsc_agent_eval/src/lsc_agent_eval/core/utils/api_client.py (1)
AgentHttpClient(16-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests (3.12)
- GitHub Check: tests (3.13)
- GitHub Check: mypy
🔇 Additional comments (3)
src/lightspeed_evaluation/core/constants.py (1)
3-5: DEFAULT_API_VERSION constant is well-introduced and consistentCentralizing the default API version here aligns with the new
APIConfig.versionfield and keeps versioning behavior explicit and configurable. No issues from this change.lsc_agent_eval/src/lsc_agent_eval/core/agent_goal_eval/agent_goal_eval.py (1)
56-60: AgentHttpClient initialization matches new versioned API designPassing
agent_endpointplus an explicitversion(with a safegetattr(..., "v1")default) andtoken_filecleanly aligns this orchestrator with the updatedAgentHttpClientsignature and CLI arguments. Looks correct as wired.src/lightspeed_evaluation/core/models/system.py (1)
7-12: APIConfig base/version split is clear and consistentImporting
DEFAULT_API_VERSIONand adding a dedicatedversionfield (withapi_basenow documented as “without version”) make the configuration model explicit and align well with the updatedAPIClientbehavior. The change is coherent and non‑disruptive within this module.Also applies to: 123-131
| @@ -1,4 +1,4 @@ | |||
| lightspeed_url: "http://localhost:8080" | |||
| lightspeed_url: "http://localhost:8080/v1/" | |||
There was a problem hiding this comment.
Please do the same change here as well.
src/generate_answers/ls_response.py
Outdated
| logging.info("Calling LightSpeed service for query '%s'", query) | ||
| response = self.client.post( | ||
| "/v1/query", | ||
| "/query", |
There was a problem hiding this comment.
Use parameterized version here.
| "messages": [{"role": "user", "content": prompt}], | ||
| "temperature": temp, | ||
| "n": n, | ||
| "response_format": {"type": "json_object"}, |
There was a problem hiding this comment.
not all responses are in dictionary/json format.. is there any impact on other metrics ? where the response is just the text with score.
There was a problem hiding this comment.
oh. I added that due to the parsing error I saw with raga & gemini, but the issue was actually caused by not enough response tokens as you suggested. Removed in the new commit.
Signed-off-by: Stephanie <yangcao@redhat.com>
yangcao77
force-pushed
the
base-url-update
branch
from
90f37b6 to
2c89878
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/generate_answers/ls_response.py (2)
18-25: Fix Pylint R0913/R0917 by makingversionandcache_dirkeyword‑only and fully typedAdding
versionpushedLSClient.__init__over the configured positional‑arg limit, which is now breaking the lint pipeline. You can fix both R0913 and R0917 cleanly by making the trailing params keyword‑only and tightening type hints:- def __init__( - self, - ls_url: str, - provider: str, - model: str, - version: str = "v1", - cache_dir: str = ".caches/llm_cache", - ): + def __init__( + self, + ls_url: str, + provider: str, + model: str, + *, + version: str = "v1", + cache_dir: str = ".caches/llm_cache", + ) -> None:The existing call sites already use keywords for
versionandcache_dir, so this should be non‑breaking while satisfying Pylint and the type‑hint guideline.
28-43: Include API version in the cache key to avoid cross‑version collisionsNow that
versionis a separate attribute andlightspeed_urltypically points at the host root (without/v1or/v2), the cache key:f"{self.url}-{self.provider}-{self.model}: {query}"will be identical for v1 and v2 of the same model on the same host. That can return stale or incorrect answers when switching versions.
Consider folding
self.versioninto the key:- return hashlib.sha256( - f"{self.url}-{self.provider}-{self.model}: {query}".encode() - ).hexdigest() + return hashlib.sha256( + f"{self.url}-{self.version}-{self.provider}-{self.model}: {query}".encode() + ).hexdigest()This keeps caches isolated per API version.
🧹 Nitpick comments (2)
src/generate_answers/eval_config.py (1)
18-21: Avoid duplicating the"v1"literal for API versionThe new
lightspeed_api_version: str = "v1"field is correct and matches the YAML, but it duplicates the version literal already defined as a constant elsewhere in the codebase. To reduce drift risk if the default ever changes, consider wiring this to the shared default, e.g.:+from lightspeed_evaluation.core.constants import DEFAULT_API_VERSION @@ - lightspeed_url: str - lightspeed_api_version: str = "v1" + lightspeed_url: str + lightspeed_api_version: str = DEFAULT_API_VERSIONsrc/generate_answers/ls_response.py (1)
68-71: Confirm base URL + version composition matches the intended contract
Clientis initialized withbase_url=ls_urland the request path is nowf"/{self.version}/query". With the current defaults (URL without version, separatelightspeed_api_version), this yieldshttp://host:port/v1/query, which is fine.However, the PR description mentions moving the version into the configured base URL. If
lightspeed_urlever includes/v1or/v2in production configs, this code would generate/v1/v1/queryor/v2/v1/query.Please double‑check the expected shape of
lightspeed_urlin configs and docs and either:
- enforce “no version in URL, use
lightspeed_api_versiononly” in documentation, or- drop the version prefix here and rely on a versioned base URL (
"/query"only).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/generate_answers/eval_config.py(1 hunks)src/generate_answers/eval_config.yaml(1 hunks)src/generate_answers/generate_answers.py(1 hunks)src/generate_answers/ls_response.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use type hints for all public functions and methods
Use Google-style docstrings for all public APIs
Use custom exceptions fromcore.system.exceptionsfor error handling
Use structured logging with appropriate levels
Files:
src/generate_answers/ls_response.pysrc/generate_answers/generate_answers.pysrc/generate_answers/eval_config.py
🧠 Learnings (2)
📚 Learning: 2025-09-01T07:25:15.021Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 35
File: pyproject.toml:65-65
Timestamp: 2025-09-01T07:25:15.021Z
Learning: The generate_answers console script entry point in pyproject.toml is intentionally not included in the wheel packages list because it's temporary and won't be packaged in future versions of the lightspeed-evaluation project.
Applied to files:
src/generate_answers/eval_config.yaml
📚 Learning: 2025-09-19T01:26:54.625Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 55
File: src/lightspeed_evaluation/core/metrics/manager.py:50-81
Timestamp: 2025-09-19T01:26:54.625Z
Learning: In the lightspeed-evaluation codebase, prefer clean code over defensive programming for controlled configuration files like system.yaml. The maintainer asamal4 prefers not to add error handling for user configuration mistakes that are unlikely to occur in controlled internal settings, to avoid code complexity.
Applied to files:
src/generate_answers/eval_config.yaml
🪛 GitHub Actions: Python linter
src/generate_answers/ls_response.py
[error] 18-18: Pylint: R0913 Too many arguments (6/5) in ls_response.py:18. Command 'make pylint'.
[error] 18-18: Pylint: R0917 Too many positional arguments (6/5) in ls_response.py:18. Command 'make pylint'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: mypy
- GitHub Check: tests (3.13)
- GitHub Check: tests (3.11)
- GitHub Check: pydocstyle
- GitHub Check: tests (3.12)
🔇 Additional comments (2)
src/generate_answers/eval_config.yaml (1)
2-2: Config:lightspeed_api_versiondefault is consistentAdding
lightspeed_api_version: "v1"here lines up with the EvalConfig default and the LSClient usage; this keeps versioning explicit and configurable.src/generate_answers/generate_answers.py (1)
169-179: Config‑driven propagation of API version into LSClient looks correctPassing
version=config.lightspeed_api_versionhere cleanly wires the new config field into LSClient and is compatible with makingversionkeyword‑only as suggested; no further changes needed at this call site.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/generate_answers/ls_response.py (1)
1-1: Fix Black formatting issue.The pipeline reports a Black formatting failure. The spacing around the comment on line 18 needs adjustment.
Run the following command to fix the formatting:
#!/bin/bash uv run black --fix src/generate_answers/ls_response.py
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/generate_answers/eval_config.py(1 hunks)src/generate_answers/eval_config.yaml(1 hunks)src/generate_answers/generate_answers.py(1 hunks)src/generate_answers/ls_response.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/generate_answers/eval_config.yaml
- src/generate_answers/eval_config.py
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use type hints for all public functions and methods
Use Google-style docstrings for all public APIs
Use custom exceptions fromcore.system.exceptionsfor error handling
Use structured logging with appropriate levels
Files:
src/generate_answers/ls_response.pysrc/generate_answers/generate_answers.py
🪛 GitHub Actions: Black
src/generate_answers/ls_response.py
[error] 1-1: Command failed: uv run black --check . — 1 file would be reformatted. Run 'uv run black --fix .' to format files.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ruff
🔇 Additional comments (2)
src/generate_answers/generate_answers.py (1)
172-179: Verify thatconfig.lightspeed_api_versionis properly defined.The LSClient instantiation passes the version parameter from the configuration. Confirm that
lightspeed_api_versionis defined in the EvalConfig class with an appropriate default value (e.g., "v1").src/generate_answers/ls_response.py (1)
70-70: Verify the versioned endpoint across the codebase.The dynamic endpoint construction using
f"/{self.version}/query"correctly implements the versioning requirement. However, manual verification is needed to ensure all API endpoint constructions use the version parameter consistently and no hardcoded/v1/or/v2/paths remain in the codebase that might cause inconsistencies.
src/generate_answers/ls_response.py
Outdated
| def __init__( # pylint: disable=R0913,R0917 | ||
| self, | ||
| ls_url: str, | ||
| provider: str, | ||
| model: str, | ||
| version: str = "v1", | ||
| cache_dir: str = ".caches/llm_cache", | ||
| ): | ||
| """Init LightSpeed.""" | ||
| self.url = ls_url | ||
| self.version = version | ||
| self.provider = provider | ||
| self.model = model |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add return type hint and update docstring per coding guidelines.
The __init__ method is missing a return type hint, and the docstring doesn't document the new version parameter. The coding guidelines require type hints for all public methods and Google-style docstrings for all public APIs.
Apply this diff to add the return type hint and update the docstring:
- def __init__( # pylint: disable=R0913,R0917
+ def __init__( # pylint: disable=R0913,R0917
self,
ls_url: str,
provider: str,
model: str,
version: str = "v1",
cache_dir: str = ".caches/llm_cache",
- ):
- """Init LightSpeed."""
+ ) -> None:
+ """Init LightSpeed.
+
+ Args:
+ ls_url: The base URL for the LightSpeed service.
+ provider: The provider name for the LLM.
+ model: The model name to use.
+ version: The API version to use (default: "v1").
+ cache_dir: Directory path for caching LLM responses.
+ """
self.url = ls_url
self.version = versionBased on coding guidelines, type hints and Google-style docstrings are required for public APIs.
🤖 Prompt for AI Agents
In src/generate_answers/ls_response.py around lines 18 to 30, the __init__
method lacks a return type hint and its docstring doesn't document the new
version parameter; add the explicit return type hint "-> None" to the __init__
signature and update the docstring to follow Google-style, including an "Args:"
section that documents ls_url, provider, model, version (with default), and
cache_dir as appropriate; keep the descriptive one-line summary and ensure
parameter types/defaults match the signature.
|
@yangcao77 While fixing the lint issue, you can also squash commits. Generally we are okay with multiple commits, but here we are adding some unwanted changes in one commit and then removing in another commit. Please squash the commits or re-organize |
Signed-off-by: Stephanie <yangcao@redhat.com>
|
@asamal4 just pushed a new commit to fix the black issue reported in CI. Re squashing commits, can we do it when merging the PR? there is an option to squash all commits into one in the PR merge option, but I do not have permission to merge any PRs, so do you mind helping merge if no other issues? |
3 participants
Description
This PR updates the code base to call
${base_url}/queryinstead of/v1/query. since lightspeed core hasv1/queryand/v2/query, our developer lightspeed is using/v2endpoints. this change to avoid hard coding thev1in code base, and user should set it as part of the base_url in configuration.also updates the docs & tests to default to
localhost:8080/v1Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.