update exceptions and guards#912
Conversation
Iroy30
added
non-breaking
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLP-only data (PDLP warm-start and reduced costs) are created and exposed only for LP problems; Solution validation helpers now receive explicit method-name strings; unknown ProblemCategory values raise ValueError; server code prefetches PDLP warm-start data before extraction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuopt/cuopt/linear_programming/problem.py (1)
1903-1907: Prefer enum-based branching inpopulate_solution.This path still hard-codes
0for LP and treats every non-LP category as MIP/IP. Consider switching to explicitProblemCategorychecks and a fail-fastelseto avoid silent misclassification if categories expand.♻️ Suggested refactor
+from cuopt.linear_programming.solver.solver_wrapper import ProblemCategory ... - self.warmstart_data = ( - solution.get_pdlp_warm_start_data() - if solution.problem_category == 0 - else None - ) + category = solution.problem_category + self.warmstart_data = ( + solution.get_pdlp_warm_start_data() + if category == ProblemCategory.LP + else None + ) IsMIP = False - if solution.problem_category == 0: + if category == ProblemCategory.LP: self.SolutionStats = self.dict_to_object(solution.get_lp_stats()) - else: + elif category in (ProblemCategory.MIP, ProblemCategory.IP): IsMIP = True self.SolutionStats = self.dict_to_object(solution.get_milp_stats()) + else: + raise ValueError( + f"Unknown problem_category: {category!r}. " + "Expected one of ProblemCategory.LP, ProblemCategory.MIP, ProblemCategory.IP." + )Also applies to: 1909-1914
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuopt/cuopt/linear_programming/problem.py` around lines 1903 - 1907, In populate_solution replace numeric category checks (e.g., solution.problem_category == 0) with explicit ProblemCategory enum comparisons (e.g., ProblemCategory.LP) when setting self.warmstart_data and the related warm-start branches around lines 1909-1914; use the enum values (e.g., ProblemCategory.LP, ProblemCategory.MIP/IP) to choose between calling solution.get_pdlp_warm_start_data() and the MIP warm-start accessor, and add a final else that raises a ValueError (or similar) for unknown categories to fail fast and avoid silent misclassification; update references to solution.problem_category, self.warmstart_data, and the relevant warm-start accessors accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 1903-1907: In populate_solution replace numeric category checks
(e.g., solution.problem_category == 0) with explicit ProblemCategory enum
comparisons (e.g., ProblemCategory.LP) when setting self.warmstart_data and the
related warm-start branches around lines 1909-1914; use the enum values (e.g.,
ProblemCategory.LP, ProblemCategory.MIP/IP) to choose between calling
solution.get_pdlp_warm_start_data() and the MIP warm-start accessor, and add a
final else that raises a ValueError (or similar) for unknown categories to fail
fast and avoid silent misclassification; update references to
solution.problem_category, self.warmstart_data, and the relevant warm-start
accessors accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
python/cuopt/cuopt/linear_programming/problem.pypython/cuopt/cuopt/linear_programming/solution/solution.py
|
/ok to test 5fff3da |
|
/ok to test ef63051 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuopt_server/cuopt_server/utils/linear_programming/solver.py (1)
307-327:⚠️ Potential issue | 🔴 CriticalFix critical callable misuse:
sol.get_pdlp_warm_start_data()invoked instead of passed as callable reference.Line 324 passes the invoked result of
sol.get_pdlp_warm_start_data()toget_if_attribute_is_valid_else_none, which expects a callable and invokes it withattr(). This causes aTypeErrorfor LP solutions (since the result is not callable) and crashes MILP success responses by raisingAttributeErrorbefore the helper can catch it. Compare with correct patterns on lines 318 and 320, wheresol.get_lp_statsandsol.get_reduced_costare passed as callable references.The code block executes for both
LPTerminationStatus.Optimal,.IterationLimit,.TimeLimitandMILPTerminationStatus.Optimal,.FeasibleFound, making this a backward compatibility violation for valid MILP solutions.Proposed patch
- pdlpwarmstart_data = get_if_attribute_is_valid_else_none( - sol.get_pdlp_warm_start_data() - ) + pdlpwarmstart_data = get_if_attribute_is_valid_else_none( + sol.get_pdlp_warm_start_data + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuopt_server/cuopt_server/utils/linear_programming/solver.py` around lines 307 - 327, The code passes the result of sol.get_pdlp_warm_start_data() into get_if_attribute_is_valid_else_none instead of the callable itself, causing a TypeError; change the call to pass the callable reference sol.get_pdlp_warm_start_data (no parentheses) so get_if_attribute_is_valid_else_none can safely invoke it, and confirm usage matches the patterns used for sol.get_lp_stats and sol.get_reduced_cost before calling extract_pdlpwarmstart_data with the returned value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@python/cuopt_server/cuopt_server/utils/linear_programming/solver.py`:
- Around line 307-327: The code passes the result of
sol.get_pdlp_warm_start_data() into get_if_attribute_is_valid_else_none instead
of the callable itself, causing a TypeError; change the call to pass the
callable reference sol.get_pdlp_warm_start_data (no parentheses) so
get_if_attribute_is_valid_else_none can safely invoke it, and confirm usage
matches the patterns used for sol.get_lp_stats and sol.get_reduced_cost before
calling extract_pdlpwarmstart_data with the returned value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1dc85df-c6b7-4136-b9d4-94c046c4885c
📒 Files selected for processing (1)
python/cuopt_server/cuopt_server/utils/linear_programming/solver.py
|
/ok to test cdc0796 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuopt_server/cuopt_server/utils/linear_programming/solver.py`:
- Around line 307-309: The call to sol.get_pdlp_warm_start_data is unguarded and
raises for MILP solutions; change the code to only invoke
get_pdlp_warm_start_data when the solved problem is an LP (i.e., check the
problem category on the solution/problem object first) and otherwise set
pdlpwarmstart_data to None; specifically, before calling
get_if_attribute_is_valid_else_none(sol.get_pdlp_warm_start_data) add a guard
that tests the problem category (e.g., compare
solution/problem.get_problem_category() or problem.category against the LP
enum/value used elsewhere) so the getter is never called on MILP paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0a0e268-e59c-41a8-b79a-a6764fff6dfd
📒 Files selected for processing (1)
python/cuopt_server/cuopt_server/utils/linear_programming/solver.py
| pdlpwarmstart_data = get_if_attribute_is_valid_else_none( | ||
| sol.get_pdlp_warm_start_data | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check LP solution API guard implementation =="
if [ -f "python/cuopt/cuopt/linear_programming/solution/solution.py" ]; then
sed -n '340,380p' python/cuopt/cuopt/linear_programming/solution/solution.py | nl -ba
else
echo "File not found at expected path"
fi
echo
echo "== Check server create_solution call site =="
if [ -f "python/cuopt_server/cuopt_server/utils/linear_programming/solver.py" ]; then
sed -n '270,335p' python/cuopt_server/cuopt_server/utils/linear_programming/solver.py | nl -ba
else
echo "File not found at expected path"
fi
echo
echo "== Find all get_pdlp_warm_start_data usages in Python files =="
rg -n -C3 '\bget_pdlp_warm_start_data\s*\(' --type=py || echo "No matches found"Repository: NVIDIA/cuopt
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check LP solution API guard implementation =="
if [ -f "python/cuopt/cuopt/linear_programming/solution/solution.py" ]; then
cat -n python/cuopt/cuopt/linear_programming/solution/solution.py | sed -n '340,380p'
else
echo "File not found at expected path"
fi
echo
echo "== Check server create_solution call site =="
if [ -f "python/cuopt_server/cuopt_server/utils/linear_programming/solver.py" ]; then
cat -n python/cuopt_server/cuopt_server/utils/linear_programming/solver.py | sed -n '270,335p'
else
echo "File not found at expected path"
fi
echo
echo "== Find all get_pdlp_warm_start_data usages in Python files =="
rg -n -C3 '\bget_pdlp_warm_start_data' --type=py || echo "No matches found"Repository: NVIDIA/cuopt
Length of output: 12731
Guard LP-only warm-start getter against MILP status paths
Lines 307-309 call sol.get_pdlp_warm_start_data() unconditionally in a code block that handles both LP and MILP termination statuses (lines 278-280). The getter raises on MILP solutions, which the wrapper get_if_attribute_is_valid_else_none() cannot catch (only catches AttributeError). This causes MILP solves to fail with an unhandled exception instead of gracefully defaulting to None. Gate the call by problem category as shown in python/cuopt/cuopt/linear_programming/problem.py:1905.
Proposed fix
+ problem_category = sol.get_problem_category()
pdlpwarmstart_data = get_if_attribute_is_valid_else_none(
- sol.get_pdlp_warm_start_data
+ sol.get_pdlp_warm_start_data
+ ) if problem_category.name == "LP" else None
- )
- solution["problem_category"] = sol.get_problem_category().name
+ solution["problem_category"] = problem_category.name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuopt_server/cuopt_server/utils/linear_programming/solver.py` around
lines 307 - 309, The call to sol.get_pdlp_warm_start_data is unguarded and
raises for MILP solutions; change the code to only invoke
get_pdlp_warm_start_data when the solved problem is an LP (i.e., check the
problem category on the solution/problem object first) and otherwise set
pdlpwarmstart_data to None; specifically, before calling
get_if_attribute_is_valid_else_none(sol.get_pdlp_warm_start_data) add a guard
that tests the problem category (e.g., compare
solution/problem.get_problem_category() or problem.category against the LP
enum/value used elsewhere) so the getter is never called on MILP paths.
|
/ok to test 0fe61d0 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuopt/cuopt/linear_programming/problem.py (1)
1913-1926:⚠️ Potential issue | 🟠 MajorHandle
problem_categoryexplicitly instead of treating every non-0value as MILP.Line 1915 and Line 1920 still encode LP as raw
0and send every other category down the MILP path. Ifproblem_categoryever carries an unexpected or newly added value,populate_solution()will silently callget_milp_stats()and drop LP-only data instead of failing fast. Please branch on the explicit category values here and raise on unknown ones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuopt/cuopt/linear_programming/problem.py` around lines 1913 - 1926, The code currently treats any non-zero solution.problem_category as MILP; update populate_solution (the block setting self.warmstart_data, IsMIP, self.SolutionStats, primal_sol, reduced_cost) to explicitly branch on known categories (e.g., 0 -> LP: call get_pdlp_warm_start_data(), get_lp_stats(), set reduced_cost from get_reduced_cost(); 1 -> MILP: set IsMIP True, call get_milp_stats(), set reduced_cost to None) and raise a clear exception if solution.problem_category is an unexpected value so we fail fast rather than silently calling get_milp_stats() for unknown categories.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 1913-1926: The code currently treats any non-zero
solution.problem_category as MILP; update populate_solution (the block setting
self.warmstart_data, IsMIP, self.SolutionStats, primal_sol, reduced_cost) to
explicitly branch on known categories (e.g., 0 -> LP: call
get_pdlp_warm_start_data(), get_lp_stats(), set reduced_cost from
get_reduced_cost(); 1 -> MILP: set IsMIP True, call get_milp_stats(), set
reduced_cost to None) and raise a clear exception if solution.problem_category
is an unexpected value so we fail fast rather than silently calling
get_milp_stats() for unknown categories.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f50f9fc2-2a4e-49ea-9389-28d9dd4da7b4
📒 Files selected for processing (1)
python/cuopt/cuopt/linear_programming/problem.py
|
/ok to test 42871b4 |
|
/merge |
2 participants
Description
Closes #870
Issue
Checklist