Skip to content

Add inference parameters to provider requests - #78

Merged
FernandoCelmer merged 3 commits into
dotflow-io:masterfrom
Versteinert:feat/inference-params
Aug 25, 2026
Merged

Add inference parameters to provider requests#78
FernandoCelmer merged 3 commits into
dotflow-io:masterfrom
Versteinert:feat/inference-params

Conversation

@Versteinert

@Versteinert Versteinert commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Description

  • .gitignore — Added entries for build artifacts (1 addition)
  • pycodeloop/cli/commands/chat.py — Added --temperature and --max-tokens CLI options to the interactive chat command, forwarded to build_flow (13 additions)
  • pycodeloop/cli/commands/run.py — Same options added to the non-interactive run command (10 additions)
  • pycodeloop/cli/flow.pyresolve_provider and build_flow updated to accept and pass temperature/max_tokens; builds inference_params dict and adds it to provider_kwargs (16 additions, 1 deletion)
  • pycodeloop/providers/generic.pyGenericProvider.__init__ accepts inference_params: dict | None; _build_from_json reads request.params from JSON config; reload() syncs inference_params; _default_request converted from @staticmethod to instance method and spreads self.inference_params into the request body (9 additions, 1 deletion)
  • tests/providers/test_generic.py — New test verifying inference_params are present in the request built by _default_request (15 additions)

Motivation and Context

Adds --temperature and --max-tokens CLI flags for user control of LLM sampling parameters without editing JSON config files. Enables JSON config files to declare static inference parameters under request.params. Improves user experience for controlling model behavior directly from the command line.

Types of changes

  • Bug fix (change that fixes an issue)
  • New feature (change which adds functionality)
  • Documentation

Checklist

  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the CHANGELOG
  • I have updated the documentation accordingly

@FernandoCelmer FernandoCelmer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Code Review

Code issues found: 2

# Severity Comment
1 [Blocking] request_params read from JSON but never passed to constructor — inference params from JSON config are silently dropped
2 [Suggestion] temperature/max_tokens typed as float/int in chat.py instead of `float

Overall the approach is solid — the is not None guard for temperature=0, the **self.inference_params placement (explicit keys win), and the reload() update all look correct. The test covering temperature=0 is a nice touch.

Comment thread pycodeloop/providers/generic.py
Comment thread pycodeloop/cli/commands/chat.py
@FernandoCelmer
FernandoCelmer self-requested a review August 18, 2026 18:50

@FernandoCelmer FernandoCelmer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Code Review

Code issues found: 3

# Severity Comment
1 [Blocking] CLI --temperature/--max-tokens silently dropped for JSON providers
2 [Blocking] **self.inference_params spread position can mask user intent
3 [Suggestion] _ConnectionSnapshot missing inference_params breaks thread-safety guarantee

Comment thread pycodeloop/cli/flow.py
Comment thread pycodeloop/providers/generic.py
Comment thread pycodeloop/providers/generic.py
@FernandoCelmer FernandoCelmer added the bug Something isn't working label Aug 24, 2026

@FernandoCelmer FernandoCelmer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Code Review

Code issues found: 3

# Severity Comment
1 [Blocking] inference_params silently dropped for JSON providers
2 [Blocking] _ConnectionSnapshot missing inference_params
3 [Suggestion] request.params double-applied with default builder

Comment thread pycodeloop/cli/flow.py
Comment thread pycodeloop/providers/generic.py
Comment thread pycodeloop/providers/generic.py
@FernandoCelmer FernandoCelmer added the enhancement New feature or request label Aug 24, 2026
…late providers

- providers/__init__.py: _from_json_path now merges CLI-provided
  inference_params on top of the ones declared in the config, instead
  of silently discarding them — the CLI flags previously had zero
  effect for any JSON config or bundled template.
- providers/generic.py: inference_params moved into _ConnectionSnapshot
  and applied to the built request body inside complete(), instead of
  being read live off self inside _default_request — closes the race
  where a concurrent reload() could hand a request a stale/mixed
  config. Also stops _build_from_json from double-applying
  request.params when a custom request_builder already bakes them in.
- Reformat with the repo's ruff config.
@FernandoCelmer

Copy link
Copy Markdown
Member

Pushed a fix on top of this branch addressing the blocking review comments:

  • providers/__init__.py: _from_json_path now merges CLI inference_params on top of the config's own — previously silently dropped for every JSON config / bundled template, which was the primary use case.
  • providers/generic.py: inference_params moved into _ConnectionSnapshot and applied to the built body inside complete(), instead of being read live off self inside _default_request — closes the reload()-race the snapshot exists to prevent.
  • _build_from_json no longer double-applies request.params when a custom request_builder already bakes them into the body statically.
  • Reformatted chat.py with the repo's ruff config (this was the CI failure on 3.13).
  • Extended tests/providers/test_generic.py to cover the merge at the complete() level and the custom-builder-plus-CLI-override case.

326/326 relevant tests pass locally (excluded test_serve_end_to_end.py, which fails the same way on feat/inference-params before this fix too — pre-existing flake, unrelated).

@FernandoCelmer
FernandoCelmer merged commit 0faeb28 into dotflow-io:master Aug 25, 2026
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants