Review tool does not update queue properly anymore

[ietf-svn-bot]

owner:sasha@dashcare.nl resolution_fixed type_defect | by kivinen@iki.fi

---

It seems that the review tool is now broken and does not update the queue at all. In my last assignments, I needed to go and manually add "Skip next" for every single person I assigned review to, as the head of the queue was not properly adjusted.

So my reviewer list before the assignment was so that the top of the list was Shawn Emery, but he had Skip 1 as I had just few minutes ago assigned him document, and did not want him to get 2nd document. The 1st person to get assignment was Stephen Farrell (see picture 1).

I went to "Manage unassigned reviews" page, clicked Not assigned to make auto assignment, and it correctly picked the first of the reviewer queue, i.e., Stephen Farrell (picture 2).

Then I clicked Save changes, and went to see how the queue is and I can see it is again wrong, now the Shawn has been correctly moved to the bottom, but the top of the list is Stephen Farrell, to whom I just assigned document. I need to go and manually adjust him, to add Skip next 1 review to make him not to be on top of the queue.

---

Issue migrated from trac:2721 at 2022-03-04 07:10:44 +0000

[ietf-svn-bot]

@kivinen@iki.fi uploaded file issue-1-before.png (132.8 KiB)

Picture 1, reviewer queue before assignment.

[ietf-svn-bot]

@kivinen@iki.fi uploaded file issue-2-assignment.png (54.1 KiB)

Picture 2, actual assignment

[ietf-svn-bot]

@kivinen@iki.fi uploaded file issue-3-after.png (106.5 KiB)

Picture 3, reviewer queue after the assignment.

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

I just ran a set of assignments through in my test database and the queue was updating as expected.

I even tried setting skip next to 1 for the first person in the queue. When I went to the manage unassigned reviews page, it did not try to assign anything to that person (it skipped him as expected).

Is there any chance you had multiple tabs up leading you to look at stale data (on the unassigned reviews page in particular)?

[ietf-svn-bot]

@kivinen@iki.fi commented

---

This issue has been there before also, but sometimes it goes away, and them comes back. Usually you can get it to appear, if you do some out of order assignments, i.e., assign some documents to some people who are NOT in first in the queue. Normally the way to fix is it to make sure you manually do skip next for every people in queue which are too early, and then do one normal in order assignment to person who is first in queue (i.e., automatic assignment). This time doing this did not fix the situation, as I tried it few times before sending the bug report.

I assume there is something wrong with the database, so it would be better to take current database dump and try it on this.

And yes, I did of course have about 50 tabs open, for which only around half a dozen are pointing to datatracker, but this is how I normally operate, and should not affect anything, as I do reload the page I do operations before clicking anything. The normal process of assigning things requires me to check multiple things on the datatracker before I can do assignments, so there is no way I can do assignments by limiting myself to only one tab....

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

I was using a fresh copy of the database.

I think I've figured out the pattern that is causing confusion though. There's nothing wrong with the database, and the code is doing what we told it to do.

Please read the rest of this while looking at https://trac.tools.ietf.org/tools/ietfdb/browser/trunk/ietf/review/utils.py#L517

When you assign a document, the queue only rotated if the assignment was to the person at the head of the queue. Otherwise the queue is left alone. (There is a exception to this if the person at the head of the queue has a non-zero skip at the time this function is called, but that can only happen if you set that skip manually, and not while assigning a document - this happens at line 553).

So, if you aren't assigning a document to the person at the head of the queue, the queue won't rotate (again unless you manually set a skip on the person at the head).

In you original report above, you had indeed manually set that skip (I assume after you did an assignment that somehow assigned him a document without causing the queue to rotate). When you assigned the document to Stephen, the queue rotation happened at line 553 (not line 536). Stephen remained at the head of the queue, though, because he did not have a non-zero skip at the time of that assignment.

So the code is doing what we told it to do. It may not be what you want yet. Perhaps it should automatically suggest incrementing the skip when you assign a document to someone that's not at the head of the queue? Or maybe we should break out the cleanup loop and run it when the assignment page is loaded? (That wouldn't cause skips to be decremented more than once on a given person unless the entire team has non-zero skips set).

[ietf-svn-bot]

@kivinen@iki.fi commented

---

The behavior has changed at some point, and what it was supposed to do and what it did was as follows:

When finding who to assign document next, skip over everybody who is not available from the beginning of queue, and skip over also everybody who has nonzero skip count. This gives the list of people who to propose assignments. This is done when the "Manage unassigned reviews" page is loaded. This works. That page correctly proposes assignments to be done. I.e., in my example it did automatically skip over Shawn and proposed Stephen for me.

When I hit the save (or save and continue) on the page, it should do bit different operation. It should now again skip over everybody who is unavailabe, but keep the person who have nonzero skip count on the list. Then it should take the first person from that list, and see if it is on the assignments done by this save. If it is, it should record that, create a review request, and move person to the end of list. If first person on the list is NOT assigned review to be done, it should check if that person has nonzero skip count, and if so, it should decrement the counter and move person to the end. After that it should take first person on the list again and do same check again. This used to work earlier, i.e., I could do out of order assignments (or more accurately I could click on auto assignment on the page in any order, first assigning last document, then 2nd, then 2nd last, then first etc, and the tool did not create in which order I did the assignments, as long as I did not skip any.

Currently this is NOT working. I.e., even when I just use automatic assignment which correctly takes person from the top of the list (Farrell), for some reason that person is not moved to the bottom of the list, but stays on the top.

If secretary does completely out of order assigment, i.e., picks someone very far from the list, or skips one person on the list, then queue only advances until that person, but does not advance from that point forward, and rest of the assignments are assumed to be out of order, and they do not affect queue order.

Example:

My reviewer list is: A, B, C, D, E.

I click manage unassigned reviews, and get 3 documents draft-x, draft-y, draft-z. I click assign button on the draft-z, and tool will automatically propose A. I will click assign button on draft-x and tool will propose B, and when I click the last draft-y the tool will propose C.

After I save, my reviewer list is: D, E, A, B, C.

Next time I do assignments I again have 3 documents, draft-u, draft-v, draft-w. I click draft-u and tool will propose D for me, but I decide to pick B for that, then I click draft-v, and tool will again propose D for me, as I have not assigned anything to him yet, so I accept that. Then I click draft-w and tool will propose E for me, and I accept that.

After I save, my reviewer list is A, B, C, D, E, i.e., even when I did assign document to B, it never was top of the list thus his review was assumed to be out of order, and he was not moved out of the list.

What currently seems to happen that if my queue is A, B, C, D, E and I assign document draft-x, draft-y, and draft-z just like I said earlier, my queue will still be A, B, C, D, E, i.e., queue was not advanced at all.

[ietf-svn-bot]

@kivinen@iki.fi commented

---

I hit this issue again, I now again end up in situation where I need manually update skip counts for every single user as I had to add skip counts for the head of the queue. Can we get this bug fixed soon?

I will try to see if I can get this resolved by adding some random unavailablity periods for some people and see if that fixes issue.

[ietf-svn-bot]

@rjsparks@nostrum.com uploaded file test_skipnext.patch (4.5 KiB)

Crufty test exercising at least one place the logic isn't right

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

Tero and I came up with the test in test_skipnext.patch at the IETF105 codesprint. It's not a useful addition to the testsuite, but it may help with understanding the issue the ticket is pointing out.

[ietf-svn-bot]

@sasha@dashcare.nl changed status from new to assigned

[ietf-svn-bot]

@sasha@dashcare.nl changed owner from `` to sasha@dashcare.nl

[ietf-svn-bot]

@sasha@dashcare.nl commented

---

So, as part of the wider Reviewer Queue Management, the first step I'm working on is extracting the current code relating to reviewer queues (roughly done), cleaning it up and seeing how the logic works. And I think this bug is a lot weirder than described so far.

Here's how I understand the structure:

a. setup_reviewer_field configures the reviewer list that Tero is talking about. The idea being that the list is kind of in order of who a review should be assigned to.
b. The main logic for prioritising reviewers is in make_assignment_choices. It scores each reviewer on things that seem logical to take into account, like conflicts and review wishes.
c. reviewer_rotation_list is used in things like the reviewer overview pages, and the mail containing open review assignments. It is also one of the inputs for make_assignment_choices. The order in which it returns reviewers has the reviewer in NextReviewerInTeam first, and then sorted by last name?
d. possibly_advance_next_reviewer_for_team is called when a review is assigned. It takes the list from reviewer_rotation_list, and then goes down the list to mark someone as NextReviewerInTeam. It passes over anyone with a skip_count, and decreases their skip_count while passing over them.

Now, a number of things seem very odd to me:

1. The prioritisation made by reviewer_rotation_list seems to place NextReviewerInTeam first, and then sort by last name? This seems like a strange choice? The lower someone is on the list, the lower the score assigned in make_assignment_choices. It looks like the further your last name is in the alphabet, the least likely you are to be assigned a review?
2. The way it's worded, NextReviewerInTeam suggests that that person should be the next reviewer. However, being in that object or not, only has a score effect of 1 in make_assignment_choices (indirectly through reviewer_rotation_list). And, possibly_advance_next_reviewer_for_team assigns NextReviewerInTeam to the first (kinda, see later) non-skipped reviewer based on the list from reviewer_rotation_list, which means NextReviewerInTeam is actually assigned to the highest person in the alphabet, that is not skipped.
3. I am also wondering whether the lists shown on the reviewer overview and the email of open review assignments are actually correct, because they are based on reviewer_rotation_list, which as far as I can see is not actually the rotation list - that list comes from make_assignment_choices.
4. The adjusting of the skip count seems to be entirely in possibly_advance_next_reviewer_for_team. The current process seems to be: start from the top of the reviewer_rotation_list output, reduce skip count by 1 for everyone with skip count > 0, the first reviewer encountered with skip count 0 is assigned in NextReviewerInTeam. Other skip counts are not decreased. Since reviewer_rotation_list is almost entirely alphabet-sorted, I think that people later in the alphabet have less chance of having their skip count decreased, i.e. be assigned fewer reviews.
5. It also seems in possibly_advance_next_reviewer_for_team that if a reviewer with skip count > 0 is assigned anyways, their skip count is decreased, though they were not skipped. However, this does not apply if they were NextReviewerInTeam (i.e. top of reviewer_rotation_list).
6. If a reviewer was not NextReviewerInTeam, and is assigned a review, it looks like they may immediately be assigned NextReviewerInTeam, even though they should obviously not be next. This may not be noticeable because the impact of NextReviewerInTeam is limited.

So here's how I think it currently works, based on reading.

With these reviewers offered in this order for the secretary (skip count in brackets):

X(2), A(2), C(0), B(1, also NextReviewerInTeam)

and the secretary assigns C, the following seems to happen:

• The skip count of X remains at 2, even though X was skipped (it is later in the alphabet than C).
• The skip count of A is correctly decreased to 1.
• The skip count of B is incorrectly decreased to 0 (it is earlier in the alphabet than C).
• C is assigned NextReviewerInTeam, as it's the first encountered reviewer in the list with skip count zero, and not the current NextReviewerInTeam.

All except the decrease in skip count for A seem wrong. I should stress that it is possible I misread the code somewhere. I'm also intrigued that an alphabet-bias, if I am correct, hasn't been noticed before.

My first idea for a solution is to at least have a single source of truth of which reviewer is next, i.e. likely remove reviewer_rotation_list because everything about it seems broken. That should help with possibly_advance_next_reviewer_for_team too. I am not sure whether there is any point to keeping NextReviewerInTeam going forward. At least it's poorly named.

Any thoughts?

[ietf-svn-bot]

@sasha@dashcare.nl commented

---

On second look, reviewer_rotation_list is even weirder, which is how the alphabet issue may not have been noticed before. The order returned is:

• First, the person in NextReviewerInTeam
• Then, everyone who by alphabetic last name order appears after that person
• Then, everyone who by alphabetic last name order appears before after that person

To give an example:

[<Person: Jari Arkko>, <Person: Stewart Bryant>, <Person: Brian Carpenter>,
<Person: Elwyn Davies>, <Person: Linda Dunbar>, <Person: Francis Dupont>,
<Person: Roni Even>, <Person: Tim Evens>, <Person: Fernando Gont>,
<Person: Vijay Gurbani>, <Person: Wassim Haddad>, <Person: Joel Halpern>,
<Person: Christer Holmberg>, <Person: Russ Housley>, <Person: Erik Kline>,
<Person: Jouni Korhonen>, <Person: Paul Kyzivat>, <Person: Henrik Levkowetz>,
<Person: Matthew Miller>, <Person: Francesca Palombini>, <Person: Pete Resnick>,
<Person: Ines Robles>, <Person: Dan Romascanu>, <Person: David Schinazi>,
<Person: Meral Shirazipour>, <Person: Robert Sparks>, <Person: Dale Worley>,
<Person: Peter Yee>]

is the alphabetic ordered list. The person in NextReviewerInTeam is Meral Shirazipour, in index 24.

The output is now (newlines added for readability):

[<Person: Meral Shirazipour>,

<Person: Robert Sparks>, <Person: Dale Worley>, <Person: Peter Yee>,

<Person: Jari Arkko>, <Person: Stewart Bryant>, <Person: Brian Carpenter>,
<Person: Elwyn Davies>, <Person: Linda Dunbar>, <Person: Francis Dupont>,
<Person: Roni Even>, <Person: Tim Evens>, <Person: Fernando Gont>,
<Person: Vijay Gurbani>, <Person: Wassim Haddad>, <Person: Joel Halpern>,
<Person: Christer Holmberg>, <Person: Russ Housley>, <Person: Erik Kline>,
<Person: Jouni Korhonen>, <Person: Paul Kyzivat>, <Person: Henrik Levkowetz>,
<Person: Matthew Miller>, <Person: Francesca Palombini>, <Person: Pete Resnick>,
<Person: Ines Robles>, <Person: Dan Romascanu>, <Person: David Schinazi>]

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

Responding to the last observation first:

That ordering of the reviewer list is expected. The algorithm we have now is rooted in Tero's original code from his management of secdir. The idea there is that assignments are made in more or less a round-robin fashion, in alphabetic-by-last-name order. The "more or less" part is where this notion of skip comes in. A reviewer might get skipped, and others might get this "skip-next" increased so they get skipped next time (because they did a really difficult review for example, and deserve a break). The team would see who was next in queue after assignments are made so they can guess roughly how long it will be until they get an assignment.

As this algorithm was ported into the datatracker, the notion of unavailability periods were added. Those perhaps in the future could possibly be used in this older algorithm in place of skip-next.

In any case, we do want to keep a roughly alphabetical-by-last-name queue for the older algorithm. However, when a group chooses a different scheduling algorithm (like the least-recently-used algorithm proposed in another ticket) preserving an last-name-ordering becomes nonsense.

Backing up to questions in comment 8:

No, I don't see the issue yet with where your last name's first letter is in the alphabet affecting your assignment ordering beyond the base queue being last-name ordered. I also think NextReviewerInTeam isn't that badly named, though I think the test in the patch shows that at some point, NextReviewerInTeam was left at a point that was obviously not the next reviewer in the queue. Specifically, the case you spelled out (starting at X(2)) is an edge case where the queue rotation happened and stopped at X while X had a skip count of zero, then someone incremented the skip count before the next assignment opportunity arose. That is to say, it is a bug if the rotation algorithm stopped leaving NextReviewerInTeam pointing at someone who had a positive skip count.