-
Notifications
You must be signed in to change notification settings - Fork 468
Fix custom LLM stream parameter handling for numeric values and persistence #462
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: master
Are you sure you want to change the base?
Conversation
- Support numeric stream values (0/1) in addition to boolean (true/false) - Preserve stream parameter in Additional request body when explicitly set - Allow stream in Additional request body to override checkbox value - Fix stream parameter parsing in LlmConfig.fromLegacyFormat() Co-authored-by: phodal <[email protected]>
Co-authored-by: phodal <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe PR extends JSON stream parameter parsing to support numeric primitives (0/1) in addition to booleans and strings, and ensures consistent streaming state handling in both test connections and model usage by prioritizing explicit body overrides over UI checkbox defaults. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
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: 0
🧹 Nitpick comments (3)
core/src/test/kotlin/cc/unitmesh/devti/llm2/model/CustomRequestTest.kt (1)
11-79: Stream parsing tests are comprehensive; only minor optional extensionsThe tests nicely exercise boolean, numeric 0/1, string, and missing
streamcases, and verify bothresult.streamand the rawbody["stream"]values, which matches the intended legacy parsing behavior. If you later want to harden this further, you could:
- Add one non‑1 numeric case (e.g.,
2) to confirm “any non‑zero” stays true.- Optionally DRY the repeated
fromLegacyFormat+ assertions via a small helper or parameterized test.These are purely optional; current coverage already looks good.
core/src/main/kotlin/cc/unitmesh/devti/llm2/model/LlmConfig.kt (1)
45-55: Numericstreamhandling looks correct; consider centralizing the parsing logicThe extended
whenbranch forJsonPrimitivecorrectly maps:
- booleans via
booleanOrNull,"true"/"false"strings viatoBoolean(),- integers/longs via
!= 0/!= 0L,which matches the new tests and the intent to support numeric 0/1 while defaulting safely to
truewhen ambiguous.The same boolean/numeric interpretation logic is now duplicated in
LLMDialog(foractualStreamin bothtestConnection()anddoOKAction()). Consider extracting a small helper (e.g., onJsonElementor inCustomRequest) to compute aBooleanfrom astreamelement so that future tweaks only need to be made in one place.core/src/main/kotlin/cc/unitmesh/devti/settings/dialog/LLMDialog.kt (1)
269-299:actualStreamderivation and usage are consistent; consider DRY’ing the shared logicThe changes in both
testConnection()anddoOKAction()form a coherent flow:
- Build
bodywith:
- Explicit
modelandtemperaturefields,- A default
streamfrom the checkbox,- Then overlay
additionalBodyso an explicitly providedstreamin JSON wins.- Derive
actualStreamfrom the finalbody["stream"], interpreting:
- booleans directly,
"true"/"false"strings viatoBoolean(),- numeric 0 as
falseand any other integer/long astrue,- falling back to the checkbox when value is absent/ambiguous.
- Use
actualStreamboth forCustomRequest.streamand to pick the default JSONPath inresponseResolverwhen the field is empty.This resolves the mismatches described in the linked issue (checkbox vs explicit body and numeric 0/1) and keeps Test Connection behavior consistent with actual dialog usage.
Given that the
actualStreamcomputation and accompanying comments are duplicated here (twice) and conceptually mirror the logic inLlmConfig.fromLegacyFormat, you might want to extract a shared helper (e.g.,fun resolveStream(streamElement: JsonElement?, fallback: Boolean): Boolean) and call it from both methods and from the legacy parser. That would reduce the chance of subtle divergence if you ever tweak the rules again.Also applies to: 303-308, 403-412, 414-427, 445-455
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/kotlin/cc/unitmesh/devti/llm2/model/LlmConfig.kt(1 hunks)core/src/main/kotlin/cc/unitmesh/devti/settings/dialog/LLMDialog.kt(4 hunks)core/src/test/kotlin/cc/unitmesh/devti/llm2/model/CustomRequestTest.kt(1 hunks)
🔇 Additional comments (1)
core/src/main/kotlin/cc/unitmesh/devti/settings/dialog/LLMDialog.kt (1)
117-121: Preservingstreamin the editable body aligns with explicit override semanticsKeeping
streaminbodyWithoutModelTemp(only filtering outmodelandtemperature) ensures that any explicitly configuredstreamvalue—especially numeric 0/1—shows up in “Additional request body (JSON)” and survives round‑trips through the dialog. This matches the intent to let advanced users override the checkbox via the raw body while still exposing model/temperature as first‑class fields.Looks good as implemented.
Custom LLM providers accepting numeric stream values (0/1) instead of boolean failed to parse correctly. Additionally, explicit stream parameters in "Additional request body" were filtered out on dialog reopen, and non-streaming requests failed despite successful connection tests.
Changes
LlmConfig.kt - Extended
CustomRequest.fromLegacyFormat()to parse numeric stream values:LLMDialog.kt - Fixed stream parameter preservation and priority:
actualStreamparsing that respects explicit body values over checkboxtestConnection()anddoOKAction()Priority:
Additional request body > CheckboxCustomRequestTest.kt - Added test coverage for boolean, numeric, string, and missing stream values.
Example Usage
User can now explicitly override checkbox with numeric values:
{ "stream": 0 }Stream parameter persists across dialog opens and correctly sets non-streaming response resolver (
$.choices[0].message.content).Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
dl.google.com/usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED -Xmx8g -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant -cp /home/REDACTED/.gradle/wrapper/dists/gradle-8.14.3-bin/cv11ve7ro1n3o1j4so8xd9n66/gradle-8.14.3/lib/gradle-daemon-main-8.14.3.jar -javaagent:/home/REDACTED/.gradle/wrapper/dists/gradle-8.14.3-bin/cv11ve7ro1n3o1j4so8xd9n66/gradle-8.14.3/lib/agents/gradle-instrumentation-agent-8.14.3.jar org.gradle.launcher.daemon.bootstrap.GradleDaemon 8.14.3(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.