Skip to content

migrate config system to pydantic_config#1915

Merged
samsja merged 5 commits intomainfrom
chore/migrate-to-pydantic-config
Mar 2, 2026
Merged

migrate config system to pydantic_config#1915
samsja merged 5 commits intomainfrom
chore/migrate-to-pydantic-config

Conversation

@samsja
Copy link
Member

@samsja samsja commented Feb 28, 2026

Summary

  • Replace the custom pydantic-settings wrapper (BaseSettings, parse_argv, toml_files inheritance) with the external pydantic_config library backed by tyro
  • Delete src/prime_rl/utils/pydantic_config.py (302 lines) and replace with a thin 50-line wrapper in src/prime_rl/utils/config.py
  • Replace allow_extras pattern in inference server with a vllm_extra: dict field on InferenceConfig
  • Remove toml_files inheritance from example SLURM configs (use @ base.toml @ slurm.toml composition instead)

Breaking Changes

Boolean negation flags changed location

The --no- prefix now attaches to the field name, not the full dotted path:

# Before
uv run trainer @ config.toml --no-model.compile

# After
uv run trainer @ config.toml --model.no-compile

vLLM extra arguments use vllm_extra dict instead of passthrough CLI args

The old system captured unknown CLI args and forwarded them to vLLM. The new system uses an explicit vllm_extra field:

# Before (unknown args passed through)
uv run inference @ config.toml --some-vllm-arg value

# After (use vllm_extra in TOML)
# In config TOML
vllm_extra = { some_vllm_arg = "value" }

Test plan

  • uv sync --all-extras resolves successfully
  • uv run pytest tests/unit/test_configs.py -v — all 58 tests pass
  • uv run python -c "from prime_rl.utils.config import BaseConfig, cli" — imports work
  • No remaining imports of prime_rl.utils.pydantic_config in codebase
  • No remaining imports of pydantic_settings in codebase

🤖 Generated with Claude Code

Replace the custom pydantic-settings wrapper (BaseSettings, parse_argv,
toml_files inheritance) with the external pydantic_config library which
provides tyro-backed CLI parsing, @ syntax for TOML files, and deep merge
of configs + CLI overrides.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Field(
description="Extra arguments to pass to vLLM. These are applied as attributes on the vLLM namespace after config translation.",
),
] = {}
Copy link

Choose a reason for hiding this comment

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

New vllm_extra field leaks into vLLM namespace

Medium Severity

The new vllm_extra field gets included in to_vllm() via the get_all_fields(self) loop, setting namespace.vllm_extra = {} (the raw dict) as a vLLM namespace attribute. This is unintended — the individual entries from the dict are correctly applied in server(), but the raw dict itself persists on the namespace. Since vllm_extra is not a recognized vLLM argument, this spurious attribute could cause issues if vLLM validates namespace attributes or uses vars(args) downstream.

Additional Locations (1)

Fix in Cursor Fix in Web

samsja and others added 4 commits February 28, 2026 06:07
…for tyro

- Switch dependency from refactor-tyro to fix-tyro branch (includes
  validators moved upstream)
- Simplify config.py by removing validators now in pydantic_config
- Fix benchmark script to use tyro subcommand syntax for optional
  nested configs (compile, ac, ac-offloading)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The benchmark script's --model.compile and --model.ac bare flags now
work without changes, thanks to upstream pydantic_config handling.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes dict[str, Any] tyro parser errors and union subcommand issues.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@samsja samsja merged commit 09a6875 into main Mar 2, 2026
9 checks passed
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.

2 participants