Skip to content
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

Merged
merged 2 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions cmd/dimacs/cmd.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package dimacs

import (
"context"
"errors"
"fmt"
"os"
Expand Down Expand Up @@ -53,15 +52,19 @@ func solve(path string) error {
}

// build solver
so := solver.NewDeppySolver(NewDimacsVariableSource(dimacs))
so := solver.NewDeppySolver()

// get solution
solution, err := so.Solve(context.Background(), solver.AddAllVariablesToSolution())
vars, err := GenerateVariables(dimacs)
if err != nil {
return fmt.Errorf("error generating variables: %s", err)
}
solution, err := so.Solve(vars)
if err != nil {
fmt.Printf("no solution found: %s\n", err)
} else {
fmt.Println("solution found:")
for _, variable := range solution.AllVariables() {
for _, variable := range vars {
fmt.Printf("%s = %t\n", variable.Identifier(), solution.IsSelected(variable.Identifier()))
}
}
Expand Down
23 changes: 5 additions & 18 deletions cmd/dimacs/dimacs_constraints.go
Original file line number Diff line number Diff line change
@@ -1,38 +1,25 @@
package dimacs

import (
"context"
"strings"

"github.com/operator-framework/deppy/pkg/deppy"
"github.com/operator-framework/deppy/pkg/deppy/constraint"
"github.com/operator-framework/deppy/pkg/deppy/input"
)

var _ input.VariableSource = &ConstraintGenerator{}
func GenerateVariables(dimacs *Dimacs) ([]deppy.Variable, error) {
varMap := make(map[deppy.Identifier]*input.SimpleVariable, len(dimacs.variables))
variables := make([]deppy.Variable, 0, len(dimacs.variables))

type ConstraintGenerator struct {
dimacs *Dimacs
}

func NewDimacsVariableSource(dimacs *Dimacs) *ConstraintGenerator {
return &ConstraintGenerator{
dimacs: dimacs,
}
}

func (d *ConstraintGenerator) GetVariables(_ context.Context) ([]deppy.Variable, error) {
varMap := make(map[deppy.Identifier]*input.SimpleVariable, len(d.dimacs.variables))
variables := make([]deppy.Variable, 0, len(d.dimacs.variables))

for _, id := range d.dimacs.variables {
for _, id := range dimacs.variables {
variable := input.NewSimpleVariable(deppy.IdentifierFromString(id))
variables = append(variables, variable)
varMap[variable.Identifier()] = variable
}

// create constraints out of the clauses
for _, clause := range d.dimacs.clauses {
for _, clause := range dimacs.clauses {
terms := strings.Split(clause, " ")
if len(terms) == 0 {
continue
Expand Down
6 changes: 1 addition & 5 deletions cmd/dimacs/dimacs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package dimacs_test

import (
"bytes"
"context"
"testing"

"github.com/operator-framework/deppy/pkg/deppy"
Expand Down Expand Up @@ -43,10 +42,7 @@ var _ = Describe("Dimacs Variable Source", func() {
problem := "p cnf 3 1\n1 2 3 0\n"
d, err := dimacs.NewDimacs(bytes.NewReader([]byte(problem)))
Expect(err).ToNot(HaveOccurred())
vs := dimacs.NewDimacsVariableSource(d)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
variables, err := vs.GetVariables(ctx)
variables, err := dimacs.GenerateVariables(d)
Expect(err).ToNot(HaveOccurred())
Expect(variables).To(HaveLen(3))

Expand Down
10 changes: 6 additions & 4 deletions cmd/sudoku/cmd.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package sudoku

import (
"context"
"fmt"

"github.com/spf13/cobra"
Expand All @@ -23,11 +22,14 @@ func NewSudokuCommand() *cobra.Command {

func solve() error {
// build solver
sudoku := &Sudoku{}
so := solver.NewDeppySolver(sudoku)
so := solver.NewDeppySolver()

// get solution
solution, err := so.Solve(context.Background())
vars, err := GenerateVariables()
if err != nil {
return err
}
solution, err := so.Solve(vars)
if err != nil {
fmt.Println("no solution found")
} else {
Expand Down
11 changes: 1 addition & 10 deletions cmd/sudoku/sudoku.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package sudoku

import (
"context"
"fmt"
"math/rand"

Expand All @@ -10,22 +9,14 @@ import (
"github.com/operator-framework/deppy/pkg/deppy/input"
)

var _ input.VariableSource = &Sudoku{}

type Sudoku struct{}

func GetID(row int, col int, num int) deppy.Identifier {
n := num
n += col * 9
n += row * 81
return deppy.Identifier(fmt.Sprintf("%03d", n))
}

func NewSudoku() *Sudoku {
return &Sudoku{}
}

func (s Sudoku) GetVariables(_ context.Context) ([]deppy.Variable, error) {
func GenerateVariables() ([]deppy.Variable, error) {
// adapted from: https://github.com/go-air/gini/blob/871d828a26852598db2b88f436549634ba9533ff/sudoku_test.go#L10
variables := make(map[deppy.Identifier]*input.SimpleVariable, 0)
inorder := make([]deppy.Variable, 0)
Expand Down
3 changes: 1 addition & 2 deletions internal/solver/bench_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package solver

import (
"context"
"math/rand"
"strconv"
"testing"
Expand Down Expand Up @@ -73,7 +72,7 @@ func BenchmarkSolve(b *testing.B) {
if err != nil {
b.Fatalf("failed to initialize solver: %s", err)
}
_, err = s.Solve(context.Background())
_, err = s.Solve()
if err != nil {
b.Fatalf("failed to initialize solver: %s", err)
}
Expand Down
4 changes: 2 additions & 2 deletions internal/solver/solve.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (
var ErrIncomplete = errors.New("cancelled before a solution could be found")

type Solver interface {
Solve(context.Context) ([]deppy.Variable, error)
Solve() ([]deppy.Variable, error)
}

type solver struct {
Expand All @@ -35,7 +35,7 @@ const (
// containing only those Variables that were selected for
// installation. If no solution is possible, or if the provided
// Context times out or is cancelled, an error is returned.
func (s *solver) Solve(_ context.Context) ([]deppy.Variable, error) {
func (s *solver) Solve() ([]deppy.Variable, error) {
result, err := s.solve()

// This likely indicates a bug, so discard whatever
Expand Down
3 changes: 1 addition & 2 deletions internal/solver/solve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package solver

import (
"bytes"
"context"
"fmt"
"testing"

Expand Down Expand Up @@ -296,7 +295,7 @@ func TestSolve(t *testing.T) {
t.Fatalf("failed to initialize solver: %s", err)
}

installed, err := s.Solve(context.TODO())
installed, err := s.Solve()

var ids []deppy.Identifier
for _, variable := range installed {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,9 @@
package input

import (
"context"

"github.com/operator-framework/deppy/pkg/deppy"
)

// VariableSource generates solver constraints given an entity querier interface
type VariableSource interface {
GetVariables(ctx context.Context) ([]deppy.Variable, error)
}

var _ deppy.Variable = &SimpleVariable{}

type SimpleVariable struct {
Expand Down
66 changes: 7 additions & 59 deletions pkg/deppy/solver/solver.go
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
}
Comment on lines 13 to 19
Copy link
Member Author

@m1kola m1kola Nov 15, 2023

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

Copy link
Member

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.


// Error returns the resolution error in case the problem is unsat
Expand All @@ -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
}
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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 Solution struct. Since now the calling code provides input variables - this seems like pointless option. So I removed it.


return solution, nil
}
Loading