Skip to content

feat(llm): add top_p, top_k, temperature sampling parameters#283

Open
ScarletCarpet wants to merge 1 commit into
alibaba:mainfrom
ScarletCarpet:feat/temperature-control
Open

feat(llm): add top_p, top_k, temperature sampling parameters#283
ScarletCarpet wants to merge 1 commit into
alibaba:mainfrom
ScarletCarpet:feat/temperature-control

Conversation

@ScarletCarpet

Copy link
Copy Markdown
Contributor

Brief:
Add three commonly-used LLM sampling parameters (top_p, top_k, temperature) with support from both CLI flags and provider config. This improves the stability of LLM outputs but requires considering the trade-offs.

Test:

  • test on deepseekv4 with temp=0.0 and top_p=0.8 and review the same code it got 80% similarity of output.
  • there is no param field on http request to LLM without customlize temp/topp/topk

Change Log:

  • Add fields to ChatRequest, ClientConfig, and both OpenAI/Anthropic clients
  • Add ApplyDefaults() to fill unset fields from ClientConfig
  • Add provider config fields for built-in and custom providers
  • Add CLI flags --top-p, --top-k, --temperature for review and scan commands
  • Add config set support for providers..top_p, top_k, temperature
  • Propagate params through agent.Args → llmloop.Deps → ChatRequest
  • Update all ChatRequest call sites (plan, main loop, compression, dedup, summary, relocation, llm test)
  • Update README.md and README.zh-CN.md with new configuration items and CLI flags
  • Add unit tests for ApplyDefaults, build params, resolver, config set, and flag parsing

Discussion:

  • Suggest deprecating the LLM config, as it duplicates the provider config
  • One way to set the temperature without this commit is via extra_body, but it is less elegant
  • Suggest deprecating other languages' READMEs, keeping only English and Chinese to reduce the maintenance workload

Description

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Related Issues

Comment thread cmd/opencodereview/shared.go
Comment thread cmd/opencodereview/flags.go Outdated
Comment thread cmd/opencodereview/review_cmd.go
Comment thread cmd/opencodereview/scan_cmd.go Outdated
Comment thread internal/diff/relocation.go Outdated
Comment thread cmd/opencodereview/config_cmd.go Outdated
Comment thread cmd/opencodereview/provider_tui.go Outdated
Comment thread internal/llm/resolver.go
Comment thread internal/llm/client.go
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 10 issue(s) in this PR.

  • ✅ 9 posted as inline comment(s)
  • 📝 1 posted as summary

📊 Posting Statistics:

  • ✅ Successfully posted: 9 comment(s)
  • ❌ Failed to post: 1 comment(s)

⚠️ Inline comments shown in summary


📄 cmd/opencodereview/scan_cmd.go (L95-L98)

⚠️ GitHub could not post this as an inline comment: Unprocessable Entity: "Line could not be resolved"

Missing input validation: There is no validation for negative values of temperature, top-p, or top-k. While LLM provider APIs will reject invalid values, adding early validation here (similar to the existing checks for --max-tools, --max-git-procs, etc.) would provide better user experience with clearer error messages. For example, temperature and top-p should typically be non-negative, and top-k should be positive when set.

Brief:
Add three commonly-used LLM sampling parameters (top_p, top_k, temperature)
with support from both CLI flags and provider config.
This improves the stability of LLM outputs but requires considering the trade-offs.

Test:
- test on deepseekv4 with temp=0.0 and top_p=0.8 and review the same code it got 80% similarity of output.
- there is no param field on http request to LLM without customlize temp/topp/topk

Change Log:
- Add fields to ChatRequest, ClientConfig, and both OpenAI/Anthropic clients
- Add ApplyDefaults() to fill unset fields from ClientConfig
- Add provider config fields for built-in and custom providers
- Add CLI flags --top-p, --top-k, --temperature for review and scan commands
- Add config set support for providers.<name>.top_p, top_k, temperature
- Propagate params through agent.Args → llmloop.Deps → ChatRequest
- Update all ChatRequest call sites (plan, main loop, compression, dedup, summary, relocation, llm test)
- Update README.md and README.zh-CN.md with new configuration items and CLI flags
- Add unit tests for ApplyDefaults, build params, resolver, config set, and flag parsing

Discussion:
- Suggest deprecating the LLM config, as it duplicates the provider config
- One way to set the temperature without this commit is via extra_body, but it is less elegant
- Suggest deprecating other languages' READMEs, keeping only English and Chinese to reduce the maintenance workload
@ScarletCarpet ScarletCarpet force-pushed the feat/temperature-control branch from 41d3678 to 9ce7288 Compare July 3, 2026 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant