-
Notifications
You must be signed in to change notification settings - Fork 22
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
if installed != nil { | ||
sort.SliceStable(installed, func(i, j int) bool { | ||
return installed[i].Identifier() < installed[j].Identifier() | ||
}) | ||
} |
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.
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.
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
b2f75f6
to
ed2aba3
Compare
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.
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 |
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 |
@stevekuznetsov do I understand correctly that you are suggesting doing a I think having order defined manually in literals (e.g. |
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. |
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). |
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? |
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. |
Is it possible to add an explicit sort to the test? If yes, add it. If no, move on. |
I think I answered it here:
If you talk about something else - I would really appreciate more details and examples. I'm having troubles understanding the expectations. |
You wrote that, then you also wrote:
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. |
@stevekuznetsov thanks for rephrasing this. I think I now better understand.
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. |
Closes #142