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

✨ Ensure solver output is deterministic #159

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

m1kola
Copy link
Member

@m1kola m1kola commented Nov 14, 2023

Closes #142

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2023
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ddaf504) 65.45% compared to head (ed2aba3) 65.52%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
+ Coverage   65.45%   65.52%   +0.06%     
==========================================
  Files          11       11              
  Lines         495      496       +1     
==========================================
+ Hits          324      325       +1     
  Misses        152      152              
  Partials       19       19              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines -304 to -308
if installed != nil {
sort.SliceStable(installed, func(i, j int) bool {
return installed[i].Identifier() < installed[j].Identifier()
})
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This bit is not related to #142, but I think non-error output should also be deterministic. Hense removing the sorting of non-error output to avoid masking potential issues.

@m1kola m1kola marked this pull request as ready for review November 15, 2023 09:22
@m1kola m1kola requested a review from a team as a code owner November 15, 2023 09:22
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 15, 2023
@m1kola m1kola changed the title ✨ Ensure solver is output is deterministic ✨ Ensure solver output is deterministic Nov 15, 2023
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
Copy link
Member

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

If there's a particular order we expect the output to be emitted in, can we assert that the output is sorted in that order in the test?

@m1kola
Copy link
Member Author

m1kola commented Nov 15, 2023

If there's a particular order we expect the output to be emitted in, can we assert that the output is sorted in that order in the test?

@stevekuznetsov that is what we now do: in internal/solver/solve_test.go I removed two calls to sort.SliceStable on output form the solver. These were masking randomness of the output. Now we assert.Equal expected output against actual and this should flag any change in the order. Or am I missing something?

@stevekuznetsov
Copy link
Member

I'm asking if we can add an explicit sort in that test to further ensure that nobody accidentally put their test data in the wrong order for the assertion

@m1kola
Copy link
Member Author

m1kola commented Nov 15, 2023

@stevekuznetsov do I understand correctly that you are suggesting doing a sort.SliceStable on the expected data for double security? I'm not sure that we can come up with a sort like this because output order depends on the input constraints. So we will most likely be required to solve problem in the test data to be able to sort expected data.

I think having order defined manually in literals (e.g. Installed: []deppy.Identifier{"a", "b", "c"}) is sufficient.

@stevekuznetsov
Copy link
Member

I'm not sure that we can come up with a sort like this because output order depends on the input constraints.

That makes sense. Is it possible to change the system in the future to be independent of the order of input? Seems like it would make things easier to reason about.

@m1kola
Copy link
Member Author

m1kola commented Nov 15, 2023

Is it possible to change the system in the future to be independent of the order of input? Seems like it would make things easier to reason about.

I'm not 100% sure. Order in some problems is important. For example, in context of operator-controller we want versions to be ordered in a certain way (from newer to older in the simplest case) and constraints will dictate that order. So maybe we can make order of some things independed from input, but there will be corelation between input and output still (I think).

@stevekuznetsov
Copy link
Member

If the order is important, then implicitly hoping that the input is ordered correctly doesn't seem like the right approach, and, furthermore, it seems like the output order is knowable (and explicit).

@m1kola
Copy link
Member Author

m1kola commented Nov 15, 2023

If the order is important, then implicitly hoping that the input is ordered correctly doesn't seem like the right approach, and, furthermore, it seems like the output order is knowable (and explicit).

  • Order might or might not be important on the problem level (e.g. input constraints influence solution)
  • On the solver level we want determinism
  • For the test inputs we know expected outputs and we explicitly define them in tests

So I don't understand - what am I missing? Could you please provide an example of what you expect to see here? Or add a code suggestion?

@stevekuznetsov
Copy link
Member

If there's a specific order that the problem expects to put data into, it should not expect that the order is implicitly correct in inputs.

If there's a specific order we expect the output to be in, we can add an explicit check for that in tests.

@stevekuznetsov
Copy link
Member

Is it possible to add an explicit sort to the test? If yes, add it. If no, move on.

@m1kola
Copy link
Member Author

m1kola commented Nov 16, 2023

Is it possible to add an explicit sort to the test?

I think I answered it here:

I'm not sure that we can come up with a sort like this because output order depends on the input constraints. So we will most likely be required to solve problem in the test data to be able to sort expected data.

I think having order defined manually in literals (e.g. Installed: []deppy.Identifier{"a", "b", "c"}) is sufficient.

If you talk about something else - I would really appreciate more details and examples. I'm having troubles understanding the expectations.

@stevekuznetsov
Copy link
Member

You wrote that, then you also wrote:

I'm not 100% sure. Order in some problems is important. For example, in context of operator-controller we want versions to be ordered in a certain way

As far as I can tell, either the output of solving is entirely dependent on input and we have no opinions on it, or it needs to be "ordered in a certain way". All I'm asking is - if either a) we can know a priori what the order will be or b) we need the order to look a certain way, we test for that explicitly by asserting that the sorted set is equal to the output.

If either is not true, then what you/ve got is perfectly sufficient.

@m1kola
Copy link
Member Author

m1kola commented Nov 16, 2023

@stevekuznetsov thanks for rephrasing this. I think I now better understand.

we can know a priori what the order will be

I'm still not an expert in this area, but as far as I understand we can not know that until we solve the input.

@m1kola m1kola added this pull request to the merge queue Nov 16, 2023
Merged via the queue into operator-framework:main with commit 11c5175 Nov 16, 2023
7 checks passed
@m1kola m1kola deleted the deterministic_unsat branch November 16, 2023 22:27
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.

"constraint not satisfiable" error message is not deterministic
3 participants