-
Notifications
You must be signed in to change notification settings - Fork 118
cuopt service correct definition for status value in lp result spec #671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThis PR refactors termination status handling from numeric codes to string-based names with validation. It introduces status name collections and a validation function in the data definition module, updates the SolutionResultData schema to enforce validated string statuses, and adds a synchronization test to verify alignment between local status collections and enums. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py (1)
949-1010: Example responses now match string-based statuses; duplicatemilp_responseis dead codeUpdating the example
lp_response/milp_responsestatusfields to"Optimal"/"FeasibleFound"keeps them consistent with the new string-based status definition and the runtime behavior validated intest_lp.Note that
milp_responseis defined twice in this module; the second definition overwrites the first, so the first is effectively dead code. Not new to this PR, but you could drop the duplicate in a follow-up to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
python/cuopt_server/cuopt_server/tests/test_lp.py(1 hunks)python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{h,hpp,py}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Verify C API does not break ABI stability (no struct layout changes, field reordering); maintain backward compatibility in Python and server APIs with deprecation warnings
Files:
python/cuopt_server/cuopt_server/tests/test_lp.pypython/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py
**/*test*.{cpp,cu,py}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*test*.{cpp,cu,py}: Write tests validating numerical correctness of optimization results (not just 'runs without error'); test degenerate cases (infeasible, unbounded, empty, singleton problems)
Ensure test isolation: prevent GPU state, cached memory, and global variables from leaking between test cases; verify each test independently initializes its environment
Add tests for algorithm phase transitions: verify correct initialization of bounds and state when transitioning from presolve to simplex to diving to crossover
Add tests for problem transformations: verify correctness of original→transformed→postsolve mappings and index consistency across problem representations
Test with free variables, singleton problems, and extreme problem dimensions near resource limits to validate edge case handling
Files:
python/cuopt_server/cuopt_server/tests/test_lp.py
🧬 Code graph analysis (1)
python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py (3)
python/cuopt_server/cuopt_server/utils/routing/data_definition.py (1)
StrictModel(23-25)python/cuopt_server/cuopt_server/utils/data_definition.py (1)
StrictModel(50-52)python/cuopt_self_hosted/cuopt_sh_client/cuopt_self_host_client.py (1)
status(985-1008)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.12, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.11, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.11, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.12, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.13, amd64, rockylinux8
- GitHub Check: checks / check-style
🔇 Additional comments (2)
python/cuopt_server/cuopt_server/tests/test_lp.py (1)
216-241: Good safeguard to keep local status-name sets in sync with enumsThis test cleanly enforces that
LP_STATUS_NAMESandMILP_STATUS_NAMEStrack the solver enums without reintroducing the CUDA/RMM-init issue indata_definition. The explicit mismatch messages will make future drift easy to diagnose.python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py (1)
813-883: Add deprecation pathway for non-string status values if prior code accepts enumsThe status-name sets and validator are logically correct, but this change breaks any code paths that construct
SolutionResultDatawith non-string statuses (enums, ints). Per coding guidelines, backward compatibility in Python APIs requires deprecation warnings. Before merging, confirm:
- Whether prior versions accepted enum/int statuses and code paths need a deprecation bridge (e.g., accept enums, coerce to
.name, warn)- If string-only is truly backward compatible across all call sites
Additionally, if surfacing allowed statuses in OpenAPI schema is desired, attach
enumviajson_schema_extrausingsorted(ALL_STATUS_NAMES).
|
Address #667 |
|
/merge |
The openapi spec for the status value in lp/mip results says "int" but it is actually a string. This change corrects that.
Unfortunately, the enums defining the values cannot be directly imported because the import causes early cuda initialization which leads to rmm errors.
So, this change uses local copies and then adds a unit test to make sure they don't drift.
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.