Skip to content

Allow None for max_val_steps and max_test_steps - #160

Merged
Vedaant-J merged 2 commits into
mainfrom
vedaant/uncapped-eval-steps
Nov 3, 2026
Merged

Vedaant-J merged 2 commits into
mainfrom
vedaant/uncapped-eval-steps

Conversation

@Vedaant-J

Copy link
Copy Markdown
Contributor

Problem

max_val_steps and max_test_steps cap how many batches validation and test evaluate. There was no way to turn the cap off: the model plan only accepted an integer from 1 to 8000, and an unset value fell back to the run-mode default (2000 / 4000 for NORMAL and BEST). Large splits were therefore always evaluated on a sample, even though the DiskGraph datamodule already treats None as "no cap".

Change

The model plan now accepts None for both fields:

optimization:
  max_val_steps:      # empty / null: evaluate the whole validation split
  max_test_steps:     # empty / null: evaluate the whole test split

None passes through validation, the trainers and the datamodule unchanged, and the eval loader yields every batch of the split. The docstring notes the cost: validation then runs over the whole split every epoch, and the split's logits are held in memory.

Back-compat

Plan value Before After
unset (???) run-mode default run-mode default (unchanged)
1–8000 cap at that many batches unchanged
None / empty rejected no cap: whole split
0, > 8000 rejected rejected (unchanged)

Existing plans parse and run exactly as before. A plan that sets None can only be read by a kumoapi that includes this change.

Testing

  • test/config/test_model_plan.py: a plan with None survives YAML and JSON round-trips and validation and stays None; an unset plan still gets 2000 / 4000.
  • test/train/test_diskgraph_nested_trainer.py: None reaches the datamodule unchanged.
  • test/train/datamodule/test_diskgraph_lightning_data.py: on a 20-row val or test split, a cap of 3 yields 3 batches (6 rows), and None yields all 10 batches (20 rows).
  • Suites run locally: the three files above and packages/kumo-api/test/test_json_serde.py, all passing (698 tests).

🤖 Generated with Claude Code

None in the model plan now means "no cap": validation and test run over
the whole split. An unset value still gets the run-mode default.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@copy-pr-bot

copy-pr-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@Vedaant-J
Vedaant-J temporarily deployed to gha-kumo-workflow-tests November 2, 2026 23:05 — with GitHub Actions Inactive
@Vedaant-J

Copy link
Copy Markdown
Contributor Author

Change map

File Function / scope What changed Why
packages/kumo-api/kumoapi/model_plan.py OptimizationPlan.max_val_steps, max_test_steps Type gains None; docstring says None evaluates the whole split and what that costs Lets users turn the eval cap off; unset still means the run-mode default
test/config/test_model_plan.py test_model_plan_eval_steps New: None survives YAML/JSON round-trip and validation; unset gets 2000 / 4000 Pins both meanings at the plan level
test/train/test_diskgraph_nested_trainer.py test_init_passes_uncapped_eval_limits New: None reaches the datamodule kwargs Pins the trainer hop
test/train/datamodule/test_diskgraph_lightning_data.py test_eval_dataloader_honors_max_steps_without_replay Was val-only with a cap of 3; now val and test, capped and None, counting rows Shows None yields the whole split and an int still caps

@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration
Commits

Reviewing files that changed from the base of the PR and between acc6ea5 and d6e3595.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review

No actionable comments were generated in the recent review.

Walkthrough

OptimizationPlan.max_val_steps and max_test_steps now accept None. Their existing positive-integer bounds and defaults remain unchanged. Tests cover YAML and JSON round-tripping, normal-mode validation, forwarding None limits to the datamodule, and capped or uncapped evaluation-loader counts.

Merge Risk: Minimal · previous review acc6e

Uncapped evaluation can proceed with the required stage counts; no issue identified here prevents merging after normal checks.

Pre-merge checks: Passed 2 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Change Map Design Review Inconclusive The required PR conversation comment beginning with <!-- kumo-pr-change-map --> is not included in the supplied review context. Repository search found only the general review guidance in `docs/deve… Provide the complete PR conversation Change map comment, or enable retrieval of the PR conversation. Then assess each proposed change against the available repository diff and linked problem.
Passed Passed checks: 2 passed
Check name Status Explanation
Pr Size Passed The pull request changes 4 files. This count does not exceed 40.
Public Resource Access Passed The pull-request diff adds or modifies no external URL, repository location, or package source location. The changes only update type annotations, documentation, and tests.
Full details: Change Map Design Review

Explanation

The required PR conversation comment beginning with &lt;!-- kumo-pr-change-map --&gt; is not included in the supplied review context. Repository search found only the general review guidance in docs/development/review.md, not the PR comment. The authoritative diff is available, but this check requires assessing the proposed Change map itself, including its rationale, stopping condition, and file-specific design claims.

Resolution

Provide the complete PR conversation Change map comment, or enable retrieval of the PR conversation. Then assess each proposed change against the available repository diff and linked problem.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 5012284 and acc6ea5.

Files selected for processing: 4
  • packages/kumo-api/kumoapi/model_plan.py
  • test/config/test_model_plan.py
  • test/train/datamodule/test_diskgraph_lightning_data.py
  • test/train/test_diskgraph_nested_trainer.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Vedaant-J
Vedaant-J temporarily deployed to gha-kumo-workflow-tests November 3, 2026 18:30 — with GitHub Actions Inactive

@Victsm Victsm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed d6e3595. Explicit None now reaches the existing uncapped validation/test loader behavior, while omitted settings retain 2000/4000 defaults. YAML/JSON round-trip and no-replay coverage are included, and the increased time/logit-memory cost is documented. No new correctness blocker found; this does not depend on #159 being fully streaming.

@Vedaant-J
Vedaant-J merged commit e2d69f9 into main Nov 3, 2026
167 of 171 checks passed
@Vedaant-J
Vedaant-J deleted the vedaant/uncapped-eval-steps branch November 3, 2026 19:57

This branch was previously deployed

1 inactive deployment
gha-kumo-workflow-tests — d6e35957 Deployed Nov 3, 2026 by Vedaant-J via import #1358
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