Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where rulesets with time enabled but no timebound constraints incorrectly accepted any from timestamp, even those outside the ruleset's valid period. The fix ensures that from timestamps before the ruleset period are adjusted to the period start, while timestamps after the period return an error.
Changes:
- Added validation to reject
fromtimestamps that fall after the ruleset's period - Improved error messages when the solver fails to find a solution, providing context about invalid timestamps
- Updated period constraint creation to handle rulesets with time enabled but no timebound assumptions
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration_tests/solve/period_test.go | Added three integration tests validating behavior with time-enabled rulesets without timebound constraints |
| puanerror/errors.go | Added new NoSolutionFound error type for solver failures |
| puan/solution_creator.go | Added error enhancement logic to provide better feedback when solve fails due to invalid timestamps |
| puan/ruleset_test.go | Added unit tests for the new isValidTime method |
| puan/ruleset_creator.go | Refactored period variable and constraint creation to support time-enabled rulesets without timebound assumptions |
| puan/ruleset.go | Added isValidTime method to validate timestamps against ruleset periods |
| internal/gateway/glpk/convert.go | Enhanced error handling to detect "mipfailed" status and return NoSolutionFound error |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
oursimon
left a comment
oursimon
left a comment
There was a problem hiding this comment.
Looks good 👍 just some thoughts about error handling when validating solutionResponse.
internal/gateway/glpk/convert.go
Outdated
| status := strings.ToLower(solution.Solutions[0].Status) | ||
| if _, ok := VALID_STATUSES[status]; !ok { | ||
| if status == "mipfailed" { | ||
| return puanerror.NoSolutionFound |
There was a problem hiding this comment.
Perhaps we also want to return the error msg here so we can check why it failed?
From the glpk-rust lib:
if mip_ret != 0 {
solution.status = Status::MIPFailed;
solution.error = Some(format!("GLPK MIP solver failed with code: {}", mip_ret));
solutions.push(solution);
continue;
}
let status = glpk::glp_mip_status(lp);
match status {
1 => {
solution.status = Status::Undefined;
solution.error = Some("Solution is undefined".to_string());
},
...
WDYT about doing something like this?
if len(solution.Solutions) != 1 {
return errors.Errorf("got %d solutions, expected 1", len(solution.Solutions))
}
if solution.Solutions[0].Error != nil {
return errors.Errorf("%w: %s", puanerror.NoSolutionFound , *solution.Solutions[0].Error)
}
status := strings.ToLower(solution.Solutions[0].Status)
if _, ok := VALID_STATUSES[status]; !ok {
return errors.Errorf("got invalid status: %s, expected one of %v", status, VALID_STATUSES)
}
There was a problem hiding this comment.
Thanks for pointing this out. I adjusted the error handling a bit. NoSolutionFound is only correct if the glpk status code is 10, and don't get that explicetly in the response. Adjusted accordingly.
|
|
||
| func (c *RulesetCreator) createPeriodConstraints(periodVariables TimeBoundVariables) error { | ||
| if len(c.timeBoundAssumedVariables) == 0 { | ||
| if c.timeDisabled() { |
| solution, | ||
| ) | ||
| } | ||
|
|
Description
For rulesets with:
It was possible to provide any
from, such as after or before the period of the ruleset. This resulted in a solution with the givenfrom.After this change:
fromis before the ruleset's period, the solution's time is the beginning of the ruleset's period.fromis after the ruleset's period, an error is returned.Also improved error messages related to invalid time, and when the solver can't find a solution.
How Has This Been Tested?
Checklist: