Repository navigation
Only feed total memory readings to the ModelManager resource estimator - #40646
schizophrenicmaniac wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
📋 Review Summary
This PR cleanly resolves #40636 by removing the spawn-time memory delta observation in ModelManager._spawn_new_model, ensuring ResourceEstimator only receives consistent total memory readings alongside the full active model snapshot during release_model. The change is well-scoped and includes a clear unit test verifying the fix.
🔍 General Feedback
- Removing the delta observation from
_spawn_new_modeleliminates the contradiction in the NNLS solver (where a bias term is fit against total memory) and prevents noisy readings from unreset peak windows during concurrent model loads. - Isolation mode profiling for unknown models remains intact via
set_initial_estimateon model release. - The new unit test
test_model_manager_observations_use_total_memoryaccurately exercises the multi-model scenario and verifies the estimator history state.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40646 +/- ##
=============================================
+ Coverage 56.16% 79.94% +23.77%
=============================================
Files 1124 488 -636
Lines 178336 82558 -95778
Branches 1489 0 -1489
=============================================
- Hits 100170 65999 -34171
+ Misses 75645 16559 -59086
+ Partials 2521 0 -2521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Assigning reviewers: R: @damccorm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
ModelManager._spawn_new_modelwas recordingpeak_during_load - baseline_snapas an observation for{tag: 1}, whilerelease_modelrecords total peak memory for the snapshot of all loaded models. SinceResourceEstimator._solvefitsΣ count·cost + bias ≈ reading, the delta rows and total rows contradict each other. With a single model loaded, both kinds even end up in the same history bucket. The delta was also noisy, because the peak window isn't reset before a load.This removes the spawn-time observation, so the estimator now only sees total memory readings with the full model snapshot from
release_model. Profiling unknown models in isolation mode is unchanged, since the initial estimate is still set on release from the isolation baseline.I added a test that loads two known models at the same time and checks that the estimator history only contains total readings for the combined snapshot. It fails on master, where a 2000 MB delta is recorded for
{model_b: 1}while 3000 MB is in use.Fixes #40636