operator-framework / deppy

Deppy: The dependency resolver for Kubernetes
Apache License 2.0
14 stars 21 forks source link

✨ Ensure solver output is deterministic #159

Closed m1kola closed 6 months ago

m1kola commented 6 months ago

Closes #142

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

m1kola commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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).

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 commented 6 months ago

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 commented 6 months ago

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

m1kola commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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.