1203855 - store a context a build ranges and indexes tested to allow to go back in a secure way

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Julien Pagès (:parkouss)]

So right now the go back implementation only store a list of build ranges, ie:

https://github.com/mozilla/mozregression/blob/aca50a0bcfd5ecb152985df9260a6433dd43463e/mozregression/bisector.py#L368

So to go back, we take the build range and run the mid build.

But this is no more ideal, since for example the skip allow to choose an arbitrary build in the range - so go back is broken on the step after the skip.

We now need to store the build range AND the build index that was used. At the least.

And we would need that also to allow implementation of bug 1160078.

Comment 1

[MikeLing]

Hi Julien,

I would like to take this one, and I will ask review only after carefully test it. Thank you!

Comment 2

[MikeLing]

(In reply to Julien Pagès (:parkouss) from comment #0)

Hi Julien, is that mean we only need to change previous_data from [] to {}?

Comment 3

[Julien Pagès (:parkouss)]

Hm, I don't think so.

Basically for now in previous_data we store build_range objects only. But we lose some context, for example as described in comment 0.

I am thinking of something like this:

class BuildHistoryContext(object):
    def __init__(self, build_range, index):
        # store the build range and the index used to bisect

And then use instances of that in the previous_data list, and reuse index and build range when we go back.

Basically this is the idea - not sure how hard it is to implement it thought, last time I looked at it I remember I was thinking that it was not so easy (can't remember why). Thanks for looking at it! If you're stuck I can have another look to the code to try to find a solution.

Comment 4

[MikeLing]

(In reply to Julien Pagès (:parkouss) from comment #3)

hmm, I got some ideal about it. maybe we could define a class which inherit from 'dict', and keep the same way to store and get(eg, className[1] = build_range and range = className[1]). But make the class can store 'build_range' object as value.  Not sure if the basic dic class is enough, I need to try it out :)

Comment 5

[MikeLing]

the check is red because I haven't rewote test for it, but I would like to get feedback from you about the method I use.  https://github.com/mozilla/mozregression/pull/342

Comment 6

[Julien Pagès (:parkouss)]

The problem is that here: https://github.com/mozilla/mozregression/pull/342/files#diff-4aa71aa30be2653b96417c9fd52256b7R376

We should store build_range AND the current index in the range (it is called mid_point in that function).

I don't see the advantage of a dict here. basically, previous_data is a structure that hold each bisection step information, so what we need is more a FIFO list (first in first out).

Let's consider the following bisection:

FIRST STEP:

[1, 2, 3, 4, 5, 6] // the numbers are arbitrary build numbers, you can think of them as date instead
 G              B  // good and bad known build
previous_data = [] // value of previous_data

mozregression will ask the user to test the mid build, here the number 4 (because we have 6 elements. 6/2 = 3 and 4 is at the index 3).
Let's say the user answer 'bad'. now we have the following:

[1, 2, 3, 4]
 G        B
previous_data = [[1, 2, 3, 4, 5, 6]] // as you can see, we added in the list the previous build range

Again, mozregression Will ask to test the mid build. Now it is 3 (same thing, 4/2=2, and 3 is at the index 2).
Let's say the user answer 'bad' again. now we have the following:

[1, 2, 3]
 G     B
previous_data = [[1, 2, 3, 4, 5, 6], [1, 2, 3, 4]]

Note that we appended the new previous build range at the end. This allow us to go back, example:
Mozregression will ask again to test the mid build - this time, the number 2.
But the user wants to go back for some reason - so he hit "back" the keyboard input. Easy, what we have to do is to pop the last item in previous data, detect the mid point from this range and use the corresponding build:

[1, 2, 3, 4]
 G        B
previous_data = [[1, 2, 3, 4, 5, 6]]

We now that our build index to test again is 3, because of the same computation: 4/2=2, so we choose 3 and make the user test that build again.

That's why all of this makes me think that we need a list - not a dict - because we are just going to push things, and pop things from something that hold the bisection states (for me, a FIFO).

-------------------------------

So, based on the previsous example, we have no issues. All is fine, we can always go back it seems - but no, because now our skip command allow to choose an arbitrary build... Example:

[1, 2, 3, 4, 5, 6]
 G              B
previous_data = []

mozregression will ask the user to test build 4. After evaluation, the user find this build neither good, neither bad - it is broken, so it have to be skipped (it can't be considered a good or bad build given what the user is testing) - this is what mozregression offer with the "skip" command. So, the user skip it - then it choose the build 2 as the next build to test (he can do that, since skip allow him to choose the next build to test). We now have:


[1, 2, 3, 5, 6]
 G           B
previous_data = [[1, 2, 3, 4, 5, 6]]

BUT we are testing the build 2 and this is not anymore the mid point! So mozregression offer the build 2 to test. let's say it is good:


[2, 3, 5, 6]
 G        B
previous_data = [[1, 2, 3, 4, 5, 6], [1, 2, 3, 5, 6]]

Ok. now the user want to go back:

[1, 2, 3, 5, 6]
 G           B
previous_data = [[1, 2, 3, 4, 5, 6]]

Here mozregression will be wrong, because it will let the user test the mid build (5/2 = 2 => the build 3) and not the build 2! This is because we don't keep build indexes in previous data.

If instead ou build_data looked like this:

previous_data = [
    // (index, build_range)
    (4, [1, 2, 3, 4, 5, 6]),
    (2, [1, 2, 3, 5, 6])
]

then we could make a back command that could work for every case. I hope it is clear! We can discuss that otherwise :)

Comment 7

[MikeLing]

oh! thank you parkouss! You explain this issue so well and make a ton of sense to me!

Comment 8

[MikeLing]

Attachment: PR for bug 1203855

Comment 9

[Julien Pagès (:parkouss)]

Comment on attachment 8681117 [details] [review]
PR for bug 1203855

Unfortunately it suffers from the same issue - it now store the index, but it is never used so the "back" command will behave the same way.

It is quite hard to find a good way to do this looking at the code - it will probably involve some refactoring. :(

Comment 10

[Julien Pagès (:parkouss)]

Attachment: fix back command after a skip

Mikeling, I found a way to do that, so I'm stealing you this bug - I hope you don't mind!

So only the last commit is interesting for this bug. The two previous ones are minor cleanup, but not related at all.

From my local tests, everything looks fine. the back command is working fully now, and I don't see any regression related to the cleaned code also.

Comment 11

[MikeLing]

No problem! good to see this bug can fixed :)

Comment 12

[William Lachance (:wlach)]

Comment on attachment 8689979 [details] [review]
fix back command after a skip

This approach makes a lot of sense to me! Thanks!

Comment 13

[Julien Pagès (:parkouss)]

Cool! Changed the code with your comments addressed, thanks for the review. :)

Landed in https://github.com/mozilla/mozregression/commit/7680d2913942157d37adca29a0af4cf9b6319801

Comment 14

[MikeLing]

Comment on attachment 8689979 [details] [review]
fix back command after a skip

oops, forget to unfalg the feedback ;)