remove explicit solver settings in python#829
Conversation
📝 WalkthroughWalkthroughAdds a new get_parameter_names() API to solver_settings in C++, exposes it via Cython, and refactors Python layers to discover and iterate solver parameters dynamically instead of using hardcoded constants. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cpp/src/math_optimization/solver_settings.cu`:
- Around line 442-459: The get_parameter_names method duplicates iteration over
int_parameters, float_parameters, bool_parameters, and string_parameters and
collects duplicate names; create a shared helper (e.g., a template function
aggregate_parameter_names or for_each_parameter in the solver_settings_t class)
that accepts a callable or output container and iterates the four parameter
vectors once to append names, using an unordered_set to filter duplicates;
replace solver_settings_t<i_t,f_t>::get_parameter_names and other similar
methods with calls to this helper (referencing int_parameters, float_parameters,
bool_parameters, string_parameters and the helper name) so all aggregation logic
and deduplication live in one place.
In `@python/cuopt_server/cuopt_server/utils/linear_programming/solver.py`:
- Around line 126-133: The loop using getattr on solver_config and
solver_config.tolerances can raise AttributeError if an attribute is missing;
update the loop (referencing solver_params, solver_config,
solver_config.tolerances and solver_settings.set_parameter) to call getattr with
a safe default (e.g., None) and/or check attribute existence before accessing so
missing parameters are skipped gracefully; ensure you only call
solver_settings.set_parameter when the retrieved param_value is not None and not
an empty string.
🧹 Nitpick comments (3)
cpp/include/cuopt/linear_programming/solver_settings.hpp (1)
96-96: Consider adding a documentation comment for the new public API method.Other public methods in this class would benefit from Doxygen-style documentation. Since this is a public header, consider adding a brief description of what
get_parameter_names()returns (e.g., aggregated list of all parameter names from int, float, bool, and string parameters).The return-by-value (vs. const reference like lines 92-95) is appropriate here since the implementation constructs a new vector.
python/cuopt/cuopt/linear_programming/solver/solver_parameters.pyx (1)
44-45: Dynamic constant generation pattern is functional but worth documenting.This pattern creates module-level constants (e.g.,
CUOPT_TIME_LIMIT = "time_limit") at import time, which is a reasonable approach for dynamic parameter discovery. However:
- The one-liner is dense and could benefit from a brief comment explaining the intent.
- If
get_solver_parameter_names()returns duplicates (as noted in the C++ review), duplicate assignments will occur (harmless but wasteful).✏️ Suggested improvement for clarity
solver_params = get_solver_parameter_names() -for param in solver_params: globals()["CUOPT_"+param.upper()] = param +# Export module-level constants CUOPT_<PARAM_NAME> for each solver parameter +for param in solver_params: + globals()["CUOPT_" + param.upper()] = parampython/cuopt_server/cuopt_server/utils/linear_programming/solver.py (1)
135-136: Redundant None check for LP_data.solver_config.This condition is already checked at line 124. The nested block (lines 135-178) is already inside the
if LP_data.solver_config is not None:block, making this check redundant.♻️ Remove redundant check
if param_value is not None and param_value != "": solver_settings.set_parameter(param, param_value) - if LP_data.solver_config is not None: - solver_config = LP_data.solver_config - try: lp_time_limit = float(os.environ.get("CUOPT_LP_TIME_LIMIT_SEC"))
| template <typename i_t, typename f_t> | ||
| const std::vector<std::string> solver_settings_t<i_t, f_t>::get_parameter_names() const | ||
| { | ||
| std::vector<std::string> parameter_names; | ||
| for (auto& param : int_parameters) { | ||
| parameter_names.push_back(param.param_name); | ||
| } | ||
| for (auto& param : float_parameters) { | ||
| parameter_names.push_back(param.param_name); | ||
| } | ||
| for (auto& param : bool_parameters) { | ||
| parameter_names.push_back(param.param_name); | ||
| } | ||
| for (auto& param : string_parameters) { | ||
| parameter_names.push_back(param.param_name); | ||
| } | ||
| return parameter_names; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the parameter definitions to see if duplicates exist
head -n 200 cpp/src/math_optimization/solver_settings.cu | tail -n 140Repository: NVIDIA/cuopt
Length of output: 7060
🏁 Script executed:
#!/bin/bash
# Search for get_parameter_names usage
rg -n "get_parameter_names" --type=cpp --type=py -B2 -A5Repository: NVIDIA/cuopt
Length of output: 866
🏁 Script executed:
#!/bin/bash
# Look for similar iteration patterns in the file to assess code duplication
rg -n "for \(auto& param : .*_parameters\)" cpp/src/math_optimization/solver_settings.cuRepository: NVIDIA/cuopt
Length of output: 961
🏁 Script executed:
#!/bin/bash
# Search for Python usage of get_parameter_names
find . -name "*.py" -type f | head -20Repository: NVIDIA/cuopt
Length of output: 1222
🏁 Script executed:
#!/bin/bash
# Search for get_parameter_names in all Python files
rg "get_parameter_names" . --type=py -B3 -A10Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if duplicates in returned vector would be problematic
# Look at how parameter names are used (e.g., in loops, sets, dicts)
rg "get_parameter_names\(\)" . -A15 --type=cpp --type=py | head -60Repository: NVIDIA/cuopt
Length of output: 1267
Refactor repeated iteration patterns over parameter types into a shared helper function.
The returned vector contains duplicate parameter names (e.g., CUOPT_TIME_LIMIT, CUOPT_NUM_GPUS, CUOPT_LOG_TO_CONSOLE, CUOPT_PRESOLVE, CUOPT_LOG_FILE, CUOPT_SOLUTION_FILE, CUOPT_USER_PROBLEM_FILE appear across multiple parameter vectors). More importantly, the pattern of iterating over int_parameters, float_parameters, bool_parameters, and string_parameters is duplicated 5+ times throughout this file (lines 131–299 alone contain multiple occurrences). Per coding guidelines, 3+ code duplications in solver components should be refactored into shared utilities.
Create a helper template that accepts a callable or uses a generic aggregation pattern to avoid repeating the four-way loop. This improves maintainability and allows deduplication to be handled consistently across all parameter aggregation methods.
🤖 Prompt for AI Agents
In `@cpp/src/math_optimization/solver_settings.cu` around lines 442 - 459, The
get_parameter_names method duplicates iteration over int_parameters,
float_parameters, bool_parameters, and string_parameters and collects duplicate
names; create a shared helper (e.g., a template function
aggregate_parameter_names or for_each_parameter in the solver_settings_t class)
that accepts a callable or output container and iterates the four parameter
vectors once to append names, using an unordered_set to filter duplicates;
replace solver_settings_t<i_t,f_t>::get_parameter_names and other similar
methods with calls to this helper (referencing int_parameters, float_parameters,
bool_parameters, string_parameters and the helper name) so all aggregation logic
and deduplication live in one place.
| for param in solver_params: | ||
| param_value = None | ||
| if param.endswith("tolerance"): | ||
| param_value = getattr(solver_config.tolerances, param) | ||
| else: | ||
| param_value = getattr(solver_config, param) | ||
| if param_value is not None and param_value != "": | ||
| solver_settings.set_parameter(param, param_value) |
There was a problem hiding this comment.
Potential AttributeError if solver_config lacks expected attributes.
This loop assumes solver_config (or solver_config.tolerances) has an attribute for every parameter in solver_params. If the server's config schema doesn't define all parameters, getattr() will raise AttributeError.
Consider using getattr() with a default value to handle missing attributes gracefully:
🛡️ Proposed fix to handle missing attributes
for param in solver_params:
param_value = None
if param.endswith("tolerance"):
- param_value = getattr(solver_config.tolerances, param)
+ param_value = getattr(solver_config.tolerances, param, None)
else:
- param_value = getattr(solver_config, param)
+ param_value = getattr(solver_config, param, None)
if param_value is not None and param_value != "":
solver_settings.set_parameter(param, param_value)🤖 Prompt for AI Agents
In `@python/cuopt_server/cuopt_server/utils/linear_programming/solver.py` around
lines 126 - 133, The loop using getattr on solver_config and
solver_config.tolerances can raise AttributeError if an attribute is missing;
update the loop (referencing solver_params, solver_config,
solver_config.tolerances and solver_settings.set_parameter) to call getattr with
a safe default (e.g., None) and/or check attribute existence before accessing so
missing parameters are skipped gracefully; ensure you only call
solver_settings.set_parameter when the retrieved param_value is not None and not
an empty string.
Iroy30
added
non-breaking
1 participant
Description
Issue
Checklist
Summary by CodeRabbit
New Features
Refactor