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

feat: assign weights in graph #351

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

feat: assign weights in graph #351

wants to merge 28 commits into from

Conversation

miparnisari
Copy link
Member

@miparnisari miparnisari commented Sep 18, 2024

Description

This is the scaffolding code required to compute weights for each node and edge. It's not meant to support every kind of model - more followup PRs will come. (For example, models with cycles are not handled).

Please review each model in the weighted_graph_builder_test.go and visualize the output in https://dreampuf.github.io/GraphvizOnline/. This will help you review the correctness of the weight assignment algorithm.

Example

image

@miparnisari miparnisari marked this pull request as ready for review September 20, 2024 00:57
@miparnisari miparnisari requested review from a team as code owners September 20, 2024 00:57
Copy link
Member

@adriantam adriantam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May not be for this PR. But it will be good idea to add unit test in some of the major method as there may be cases not covered in the TestWeightedGraphBuilder test

pkg/go/graph/weighted_graph.go Outdated Show resolved Hide resolved
pkg/go/graph/weighted_graph.go Outdated Show resolved Hide resolved
pkg/go/graph/weighted_graph_builder_test.go Show resolved Hide resolved
pkg/go/graph/weighted_graph_builder.go Show resolved Hide resolved
@elbuo8
Copy link
Member

elbuo8 commented Sep 23, 2024

Why not use gonum's weighted graph? 🤔


//nolint: cyclop
func NewWeightedAuthorizationModelGraph(model *openfgav1.AuthorizationModel) (*WeightedAuthorizationModelGraph, error) {
g, err := NewAuthorizationModelGraph(model)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the existing builder?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing builder's job is to figure out the edges, their directions, etc. This builder's job is to assign weights. Since both are fairly complex jobs i'd like to keep them separate. Makes testing easier and less code per file

@miparnisari
Copy link
Member Author

miparnisari commented Sep 23, 2024

Why not use gonum's weighted graph? 🤔

@elbuo8 several reasons:

  1. gonum's weighted graph allows natively assigning weights to edges only
  2. those weights are float64 only: https://pkg.go.dev/gonum.org/v1/gonum/graph#WeightedLine

@miparnisari miparnisari requested a review from a team as a code owner September 23, 2024 19:21
@senojj
Copy link

senojj commented Sep 25, 2024

Non-Blocking

The relationship between the weighted graph builder and the weighted graph is a bit odd. Currently the weighted graph has a dependency of its builder, which seems backwards.

Instead of coupling it all together with the current approach:

weightedGraph, err := NewWeightedAuthorizationModelGraph(model)

I would suggest decoupling them explicitly:

weightedGraphBuilder := NewWeightedAuthorizationModelGraphBuilder()
weightedGraph, err := weightedGraphBuilder.Build(model)

I know this is introducing an additional step/line of code, however I really don't see the point of having the builder, otherwise.

Comment on lines 14 to 15
// isNested signals that this edge is a self-loop. The associated node will also have this flag set to true.
isNested bool
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put in other words, is this an edge that connects a vertex to itself?

  graph TD;
      A-->A;
Loading

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! not sure what to call it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants