Repository navigation
chore(configs): drop unused config knobs - #3985
Closed
faresobeid wants to merge 2 commits into
Closed
faresobeid wants to merge 2 commits into
faresobeid wants to merge 2 commits into
Conversation
…d the num_eval_* aliases
faresobeid
marked this pull request as ready for review
October 10, 2026 17:40
|
|
||
| log: LogConfig = LogConfig() | ||
|
|
||
| output_dir: Path = Field(default_factory=default_output_dir) |
Member
There was a problem hiding this comment.
u sure this is not set? env servers also log to output dir no?
| url: str | ||
| """URL to send the heartbeat to.""" | ||
|
|
||
| min_interval: float = Field(30.0, gt=0) |
| length: Literal["fixed", "variable"] = "fixed" | ||
| """Use fixed-length samples or variable-length samples.""" | ||
|
|
||
| input_ids: Literal["increasing", "random"] = "increasing" |
Member
There was a problem hiding this comment.
i feel like random might be useful?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes config knobs that nothing reads or that nobody sets:
env_server.output_dir: no reader.src/prime_rl/entrypoints/env_server.pynever uses it, and the launchers do not write it (write_env_server_configwrites onlyenv,serve,address_fileandlog).heartbeat.min_interval: no config, test or doc sets it. It becomes the constantMIN_INTERVAL_S = 30.0insrc/prime_rl/utils/heartbeat.py, the value that fix(utils): throttle BetterStack heartbeat pings #3715 picked for Better Stack's rate limit.deployment.num_eval_gpus/deployment.num_eval_nodesaliases on SFT: both arrived together withnum_infer_gpus/num_infer_nodesin feat: online evals during sft #3256, so they were never a legacy name. Nothing references them.data.input_ids: fake data now always uses theincreasingids, which was already the config default. Therandombranch and the now-unusedvocab_sizeargument ofFakeDatasetare removed, so no config changes behavior.increasingis the simpler path because it makes no generator draws. The exact-order dataloader tests already rely on it. One limitation already existed before this PR: each sample's token id is its sample index, so a very long fake-data run eventually goes past the vocab size.I re-checked the other candidates from the config audit and left them out:
dynamo.enabled: open feat(trainer): support VLM training with dynamo #3483, feat(inference): launch managed Dynamo workers #3493, feat(dynamo): support python based dynamo vLLM workers administration #3629 and Add ModelExpress public-client weight broadcast #3890 read or validate it, e.g. by rejectingenabled = falsefor managed Dynamo.rollout_transport.hwm: refactor: orchestrator as hook-wired components with per-component configs #3652 derives it frommax_off_policy_steps.data.fake.generate_samples: still read (src/prime_rl/trainer/rl/data.py:77-87), and feat: integrate DSpark draft training, weight sync, and checkpoints #3729 sets it in an example.Test plan (all run):
pytest tests/unit/orchestrator tests/unit/test_configs.py tests/unit/train/sft tests/unit/utils -m "not gpu": 482 passed. Fake-dataset tests were updated for the removed arguments.Heartbeat.beat()calls send one ping;num_eval_gpusis now rejected andnum_infer_gpusstill works;EnvServerConfig(output_dir=...)is rejected.ruff checkandruff format --checkon the changed files.Note
Low Risk
Config surface and test-only fake-data paths shrink with no production readers of the removed fields; default fake-data behavior matches the former
increasingdefault.Overview
Removes several unused or redundant config knobs and aligns runtime code with the slimmer schemas.
Config removals:
env_server.output_dir(never read by the env-server entrypoint or launchers);heartbeat.min_interval(replaced by a fixed 30s throttle inHeartbeatviaMIN_INTERVAL_S); SFT deployment aliasesnum_eval_gpus/num_eval_nodes(onlynum_infer_*remain); and fake SFTdata.input_ids.Fake SFT data:
FakeDatasetalways emits deterministic “increasing” token ids (sample index repeated per position); the random-id path andvocab_sizeplumbing are dropped. Variable-length fake data still randomizes sequence length only.Docs on eval heartbeat behavior now refer to the hard-coded 30s interval. Unit tests were updated for the new
FakeDatasetAPI and configs.Reviewed by Cursor Bugbot for commit 5fbf89e. Bugbot is set up for automated code reviews on this repo. Configure here.