Merged
Conversation
kevalmorabia97
force-pushed
the
kmorabia/remove-mip
branch
from
7ea34bc to
3a876a7
Compare
Collaborator
Author
|
Review from from author of Pulp package
|
kevalmorabia97
force-pushed
the
kmorabia/remove-mip
branch
from
3a876a7 to
8a6fc7b
Compare
Contributor
📝 WalkthroughWalkthroughThe pull request migrates the optimization solver in the puzzletron module from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
kevalmorabia97
changed the base branch from
feature/compress
to
feature/puzzletron
kevalmorabia97
force-pushed
the
kmorabia/remove-mip
branch
2 times, most recently
from
c41025b to
d65c324
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #663 +/- ##
==========================================
+ Coverage 72.54% 74.47% +1.93%
==========================================
Files 459 462 +3
Lines 48649 51360 +2711
==========================================
+ Hits 35290 38248 +2958
+ Misses 13359 13112 -247
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
modelopt/torch/puzzletron/mip/mip_with_multi_layer_replacements.py (1)
115-115: Preferpulp.value(...)over.varValuehere.PuLP exposes
value()/.value()as a documented way to read solved variable values, so this is a good place to avoid depending on the rawvarValueattribute directly. That also matches the package author's note on this PR. (github.com)🧹 Suggested cleanup
- is_chosen = replacement["is_chosen"].varValue >= 0.99 + chosen_value = pulp.value(replacement["is_chosen"]) + is_chosen = chosen_value is not None and chosen_value >= 0.99🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/torch/puzzletron/mip/mip_with_multi_layer_replacements.py` at line 115, Replace direct access to the PuLP variable attribute varValue with the documented pulp.value(...) call: where the code checks replacement["is_chosen"].varValue >= 0.99, call pulp.value(replacement["is_chosen"]) >= 0.99 instead and ensure pulp is imported in mip_with_multi_layer_replacements.py; update any equivalent occurrences that read .varValue (e.g., in the loop handling replacement["is_chosen"]) to use pulp.value or the variable's .value() accessor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelopt/torch/puzzletron/mip/mip_with_multi_layer_replacements.py`:
- Around line 91-95: The model-building code skips NaN/inf bounds by checking
math.isfinite for max_cost/min_cost when adding constraints (using problem and
constraint_vars), but the later post-solve verification still treats any
non-None bound as active and can assert on non-finite values; update the
verification loop to mirror the same normalization (treat non-finite bounds as
if they were None) before performing assertions—i.e., in the post-solve check
for max_cost/min_cost (and where constraint_key is used) only enforce the bound
if math.isfinite(bound), otherwise skip that check.
- Around line 100-105: The code currently discards solutions unless
problem.status == pulp.LpStatusOptimal; after calling solver via
pulp.PULP_CBC_CMD and problem.solve(solver) (see variables solver and problem),
change the feasibility check to also accept the best incumbent reported by PuLP
by inspecting problem.sol_status for pulp.LpSolutionIntegerFeasible (in addition
to pulp.LpStatusOptimal). In other words, after problem.solve(solver) treat the
solution as usable if problem.status == pulp.LpStatusOptimal OR
problem.sol_status == pulp.LpSolutionIntegerFeasible; otherwise handle it as
infeasible/failure. Ensure you reference pulp.PULP_CBC_CMD, problem.solve,
problem.status, problem.sol_status, pulp.LpStatusOptimal and
pulp.LpSolutionIntegerFeasible when making the change.
---
Nitpick comments:
In `@modelopt/torch/puzzletron/mip/mip_with_multi_layer_replacements.py`:
- Line 115: Replace direct access to the PuLP variable attribute varValue with
the documented pulp.value(...) call: where the code checks
replacement["is_chosen"].varValue >= 0.99, call
pulp.value(replacement["is_chosen"]) >= 0.99 instead and ensure pulp is imported
in mip_with_multi_layer_replacements.py; update any equivalent occurrences that
read .varValue (e.g., in the loop handling replacement["is_chosen"]) to use
pulp.value or the variable's .value() accessor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 63a4fee5-6cb2-4957-b8ae-992797784640
📒 Files selected for processing (3)
.github/workflows/gpu_tests.ymlmodelopt/torch/puzzletron/mip/mip_with_multi_layer_replacements.pypyproject.toml
💤 Files with no reviewable changes (2)
- .github/workflows/gpu_tests.yml
- pyproject.toml
Qwen3-8B: MIP Solver Comparison (PuLP vs MIP)Comparison of block configurations and MMLU benchmarks when solving the Puzzletron compression problem with two different MIP solver libraries.
Block Configuration DifferencesOnly 2 of 36 blocks differ between the two solvers.
Note: the PuLP solution shifts FFN capacity from Full block configuration (MIP — old)Full block configuration (PuLP — new)MMLU Benchmark Results (80% memory target)
Summary
|
kevalmorabia97
force-pushed
the
feature/puzzletron
branch
from
18e6fa6 to
41b8ca7
Compare
kevalmorabia97
force-pushed
the
feature/puzzletron
branch
from
2669a26 to
977d60a
Compare
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
kevalmorabia97
force-pushed
the
kmorabia/remove-mip
branch
from
d65c324 to
833ec9c
Compare
Contributor
|
Nemotron-Nano-12B-v2 compressed from 49000MiB to 34000MiB (memory target) with Puzzle: MIP Solver Comparison (PuLP vs MIP)Comparison of block configurations and MMLU benchmarks when solving the Mixed Integer Programming problem with two different libraries at a 34000 MiB memory target:
Block Configuration DifferencesOnly 4 of 62 blocks differ between the two solvers.
Note: the PuLP solution effectively swaps Full block configuration (MIP — old)Full block configuration (PuLP — new)MMLU Benchmark Results (Puzzle 34000 MiB)
Summary
|
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
2 participants
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.
Replace mip package with more popular pulp package for puzzle mip solving. Both use the CBC solver under the hood
Testing
Summary by CodeRabbit