-
Notifications
You must be signed in to change notification settings - Fork 21
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
⚠️ Remove variable sources and make Deppy accept a slice of variables instead #160
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,21 @@ | ||
package solver | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
|
||
"github.com/operator-framework/deppy/internal/solver" | ||
"github.com/operator-framework/deppy/pkg/deppy" | ||
"github.com/operator-framework/deppy/pkg/deppy/input" | ||
) | ||
|
||
// TODO: should disambiguate between solver errors due to constraints | ||
// and other generic errors (e.g. entity source not reachable, etc.) | ||
// and other generic errors | ||
|
||
// Solution is returned by the Solver when the internal solver executed successfully. | ||
// A successful execution of the solver can still end in an error when no solution can | ||
// be found. | ||
type Solution struct { | ||
err error | ||
selection map[deppy.Identifier]deppy.Variable | ||
variables []deppy.Variable | ||
} | ||
|
||
// Error returns the resolution error in case the problem is unsat | ||
|
@@ -40,66 +37,21 @@ func (s *Solution) IsSelected(identifier deppy.Identifier) bool { | |
return ok | ||
} | ||
|
||
// AllVariables returns all the variables that were considered by the solver to obtain (or not) | ||
// a solution. Note: This is only be present if the AddAllVariablesToSolution option is passed in to the | ||
// Solve call that generated the solution. | ||
func (s *Solution) AllVariables() []deppy.Variable { | ||
return s.variables | ||
} | ||
|
||
type solutionOptions struct { | ||
addVariablesToSolution bool | ||
} | ||
|
||
func (s *solutionOptions) apply(options ...Option) *solutionOptions { | ||
for _, applyOption := range options { | ||
applyOption(s) | ||
} | ||
return s | ||
} | ||
|
||
func defaultSolutionOptions() *solutionOptions { | ||
return &solutionOptions{ | ||
addVariablesToSolution: false, | ||
} | ||
} | ||
|
||
type Option func(solutionOptions *solutionOptions) | ||
|
||
// AddAllVariablesToSolution is a Solve option that instructs the solver to include | ||
// all the variables considered to the Solution it produces | ||
func AddAllVariablesToSolution() Option { | ||
return func(solutionOptions *solutionOptions) { | ||
solutionOptions.addVariablesToSolution = true | ||
} | ||
} | ||
|
||
// DeppySolver is a simple solver implementation that takes an entity source group and a constraint aggregator | ||
// DeppySolver is a simple solver implementation that takes a slice of variables | ||
// to produce a Solution (or error if no solution can be found) | ||
type DeppySolver struct { | ||
variableSource input.VariableSource | ||
} | ||
type DeppySolver struct{} | ||
|
||
func NewDeppySolver(variableSource input.VariableSource) *DeppySolver { | ||
return &DeppySolver{ | ||
variableSource: variableSource, | ||
} | ||
func NewDeppySolver() *DeppySolver { | ||
return &DeppySolver{} | ||
} | ||
|
||
func (d DeppySolver) Solve(ctx context.Context, options ...Option) (*Solution, error) { | ||
solutionOpts := defaultSolutionOptions().apply(options...) | ||
|
||
vars, err := d.variableSource.GetVariables(ctx) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
func (d DeppySolver) Solve(vars []deppy.Variable) (*Solution, error) { | ||
satSolver, err := solver.NewSolver(solver.WithInput(vars)) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
selection, err := satSolver.Solve(ctx) | ||
selection, err := satSolver.Solve() | ||
if err != nil && !errors.As(err, &deppy.NotSatisfiable{}) { | ||
return nil, err | ||
} | ||
|
@@ -116,9 +68,5 @@ func (d DeppySolver) Solve(ctx context.Context, options ...Option) (*Solution, e | |
solution.err = unsatError | ||
} | ||
|
||
if solutionOpts.addVariablesToSolution { | ||
solution.variables = vars | ||
} | ||
Comment on lines
-119
to
-121
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously variables were generated by a varaible soruce. So for cases where the calling code for some reason needed to access input variables - we had an option to add them into the |
||
|
||
return solution, nil | ||
} |
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.
I think we can get rid of
Solution
completely (not in this PR). I do not see a lot of value in it.And probably replace
DeppySolver
wrapper around internal package by exporting what we currently have in internal pacakge (with some modifications and also not in this PR).This comment is for me to look into whether it makes sense and create issues for this.
Edit: created #161 for this
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.
Structuring the logic to be helpers in generating the inputs to and consuming the outputs of the solver library sounds like a really good idea - lets folks use as much of or as little of this helpful logic as they need and doesn't bind them to what we happen to have or not to have wrapped at some moment in time.