remove deprecated params #770
Conversation
Iroy30
added
non-breaking
📝 WalkthroughWalkthroughRemoves deprecated parameters and fields: drops Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
python/cuopt_self_hosted/cuopt_sh_client/cuopt_self_host_client.py (1)
297-311: Breaking API change:request_excess_timeoutparameter removed from constructor.Existing code that instantiates
CuOptServiceSelfHostClientwithrequest_excess_timeout=...will now raise aTypeError. Per coding guidelines, Python APIs should maintain backward compatibility with deprecation warnings before removal.Consider accepting
request_excess_timeoutas a**kwargsparameter and emitting aDeprecationWarningif provided, or document this breaking change in release notes.Suggested approach for backward compatibility
def __init__( self, ip: str = "0.0.0.0", port: str = "5000", use_https: bool = False, self_signed_cert="", polling_interval=1, only_validate=False, polling_timeout=600, timeout_exception=True, result_type=mime_type.MSGPACK, http_general_timeout=30, data_send_timeout=-1, result_receive_timeout=-1, + **kwargs, ): + if "request_excess_timeout" in kwargs: + import warnings + warnings.warn( + "request_excess_timeout is deprecated and ignored. " + "Use polling_timeout instead.", + DeprecationWarning, + stacklevel=2 + ) self.timeout_exception = timeout_exceptionpython/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py (1)
18-20:Extra.forbidconfiguration causes hard failures for deprecated fields.With
extra = Extra.forbid, any request containing the removed field names (e.g.,solver_mode,heuristics_only,absolute_primal, etc.) will receive a422 Unprocessable Entityerror. This is a breaking change for clients still using deprecated field names.Per coding guidelines, consider temporarily changing to
Extra.ignorewith logged warnings for unknown fields, or use Pydantic validators to accept and warn on deprecated field names before full removal.python/cuopt_self_hosted/cuopt_sh_client/thin_client_solver_settings.py (1)
156-170: Old tolerance parameter names silently break clients without deprecation warning.Users passing old names (e.g.,
absolute_primal,relative_gap,integrality_tolerance) toset_parameter()will have these values placed at the top level of the solver config instead of inside the"tolerances"sub-dictionary. The server rejects these misplaced parameters due to strict validation (Extra.forbid), causing requests to fail. No deprecation warning is emitted by the thin client to alert users of this breaking change.Per coding guidelines, implement deprecation warnings when old parameter names are detected, or provide transparent mapping to new names before full removal. The
dep_warning()mechanism exists in the server codebase and should be replicated in the thin client.
🧹 Nitpick comments (1)
python/cuopt_server/cuopt_server/utils/linear_programming/solver.py (1)
64-68: Remove the unuseddep_warningfunction.The function defined at lines 64-68 is never called anywhere in the codebase and can be safely removed to reduce code clutter.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
python/cuopt_self_hosted/cuopt_sh_client/cuopt_self_host_client.pypython/cuopt_self_hosted/cuopt_sh_client/thin_client_solver_settings.pypython/cuopt_server/cuopt_server/utils/linear_programming/data_definition.pypython/cuopt_server/cuopt_server/utils/linear_programming/solver.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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_self_hosted/cuopt_sh_client/cuopt_self_host_client.pypython/cuopt_server/cuopt_server/utils/linear_programming/data_definition.pypython/cuopt_server/cuopt_server/utils/linear_programming/solver.pypython/cuopt_self_hosted/cuopt_sh_client/thin_client_solver_settings.py
🧠 Learnings (2)
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to docs/**/*.{rst,md} : Document numerical tolerances, parameter ranges, and algorithm phase descriptions in API documentation when solver behavior or tolerances change
Applied to files:
python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.pypython/cuopt_server/cuopt_server/utils/linear_programming/solver.pypython/cuopt_self_hosted/cuopt_sh_client/thin_client_solver_settings.py
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms
Applied to files:
python/cuopt_server/cuopt_server/utils/linear_programming/solver.py
🔇 Additional comments (7)
python/cuopt_self_hosted/cuopt_sh_client/thin_client_solver_settings.py (1)
151-182: LGTM on thetoDict()method structure.The logic for separating tolerance keys from non-tolerance keys and placing them in the appropriate locations is clear and correct.
python/cuopt_self_hosted/cuopt_sh_client/cuopt_self_host_client.py (1)
361-361: Simplified timeout assignment is correct.The direct assignment
self.timeout = polling_timeoutis cleaner than the previous combined logic.python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py (2)
322-374: Tolerances model now uses consistent*_tolerancenaming.The field naming is now consistent (e.g.,
absolute_primal_toleranceinstead ofabsolute_primal). The model structure is clean and well-documented.
377-569: SolverConfig model is well-structured after deprecated field removal.The remaining configuration options are properly typed and documented. The removal of
solver_modein favor ofpdlp_solver_modeandheuristics_onlyin favor ofmip_heuristics_onlymakes the naming more explicit.python/cuopt_server/cuopt_server/utils/linear_programming/solver.py (3)
170-176: LGTM onpdlp_solver_modehandling.The solver mode is now set directly from
solver_config.pdlp_solver_modewithout the deprecatedsolver_modefallback logic.
222-288: LGTM on tolerance parameter handling.The tolerance configuration now uses a single, consistent code path for each tolerance type with the
*_tolerancenaming convention.
295-298: LGTM onmip_heuristics_onlyhandling.The heuristics configuration now uses only the explicit
mip_heuristics_onlyfield.
|
/ok to test cb1c639 |
|
/ok to test c5a2c6e |
|
/ok to test 1c8d6fa |
tmckayus
left a comment
tmckayus
left a comment
There was a problem hiding this comment.
lgtm as long as we have double-checked that it has been deprecated for 2 releases
2 participants
Description
Issue
Checklist
Summary by CodeRabbit
Breaking Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.