Consider previous matches in matching process to reduce duplicate pairs

[preyansh98]

This PR seeks to target the ongoing issue of #56.

During the matching process, sometimes users get paired with the same user that they have been matched with "recently". This PR seeks to optimize the current matching algorithm to minimize the occurrence of this experience.

Basic Idea:

• Leverage the already existing attribute "RecentPairUps" in UserInfo to store a record of the last 3 users that the user has been paired with.
• In process of matching users, pick a user X. Then find a user Y in the rest of this list (queue) such that X has not been matched with Y recently.
• Update this attribute "RecentPairUps" everytime a new pair is made. We will only store up to the most recent 3 users that the user has been matched with.

[ghost]

All CLA requirements met.

[madshaun1984]

Testing this, I've found it has bugs.

If no previous pairing entries are found, a null ref occurs in "SamePairNotCreatedRecently". (I've confirmed and worked around this by adding checks for null on pairUserOneInfo & pairUserTwoInfo, before entering the loop containing a call to "SamePairNotCreatedRecently").

I believe it also needs logic to handle teams with 4 or less people.

[preyansh98]

@madshaun1984 Thanks for the review! I'll definitely add the required null checks and ping you again for your review once I've done so.

For teams with 4 or less people, I believe the logic would just default to the same? From the algorithm, we won't be able to find "perfect" pairings for the users so we'll just be picking users in groups of 2 as the old algorithm used to.

[preyansh98]

• Addressed null ref checks, and also externalized maxRecentPairUps to a configuration setting.

@madshaun1984

[gdherbert]

Not a review - but does this need to address the other calls to SetUserInfoAsync() in the code to add the additional parameter?

[preyansh98]

Hi @gdherbert,

No, I've overloaded the SetUserInfoAsync() method for the additional parameter, so existing calls will be unaffected and will continue with the same behavior 👍

[gdherbert]

I have been testing this and getting an error
"Error pairing up team members: Object reference not set to an instance of an object."

I am not sure what is causing this, but I notice that the database only contains users who have opted out (or back in) and I wonder what happens if the user doesn't exist when this gets called:
pairUserOneInfo.RecentPairUps = new List();

[TobiTh]

Could we maybe get an update on this topic? The duplicates are still very annoying :(

[markusd1984]

This feature request here to avoid duplicate/repeat matches as well as handling uneven lists are quite important issues and will be great if these get sorted out to use the icebreaker on a regular basis, till then these are quite some impacting limitations/issues.

Edit: @preyansh98 @gdherbert have you done any more testing or changes on this?

[markusd1984]

@madshaun1984 have you been able to test / use any further with success?

Anybody else tested this?

[preyansh98]

Hi @markusd1984 , thanks for the interest in this topic!

Unfortunately, haven't been able to test as I don't have an Azure provision ready to deploy and test the bot with.

If you can assist with testing, I can make the necessary dev changes to make this PR merge-ready and/or welcome any contributions to achieve that

[markusd1984]

Hi @markusd1984 , thanks for the interest in this topic!

Unfortunately, haven't been able to test as I don't have an Azure provision ready to deploy and test the bot with.

If you can assist with testing, I can make the necessary dev changes to make this PR merge-ready and/or welcome any contributions to achieve that

Thanks @preyansh98, I will see if we can test this one, need to figure out how I can get your changes applied to my fork.... just wondering if you were able to consider/review the last issue raised by @gdherbert if that's a concern?

I have been testing this and getting an error
"Error pairing up team members: Object reference not set to an instance of an object."

I am not sure what is causing this, but I notice that the database only contains users who have opted out (or back in) and I wonder what happens if the user doesn't exist when this gets called:
pairUserOneInfo.RecentPairUps = new List();

[preyansh98]

@markusd1984

I believe the error @gdherbert caught relates to the user object itself being null. Unfortunately, this would also default to an error with the old matching process so I'm not sure if a fix for that belongs in this issue/PR.

Although, I think I will catch that exception so we at least handle it gracefully. Let me know if you suggest otherwise!

Thanks

[amannall]

Can anyone advise on where this is.

I have a reasonably large Icebreaker team (250 users) but we are getting duplicates and I'd like to reduce the possibility of duplicate pairings.

Thanks

[JoMac72]

Hey, did anyone find a solution to this problem? Thanks

[markusd1984]

@amannall @JoMac72 have you tried applying the 4 commit changes to your own fork / icebreaker code to test this threads feature?

[amannall]

@markusd1984 the problem is that the MS branch is 21 commits ahead (v2) and when i compared the code in the preyansh98 branch to the MS code there are radical differences in how it now works.

[corylk]

@amannall @JoMac72 I applied preyansh's changes to the latest codebase on my own fork, made some improvements, and added more telemetry.

I'm not sure if this is how this is how the maintainers want to deal with this PR, but they can take it or leave it. I opened a PR for reference, if nothing else.

#169

[markusd1984]

@amannall @JoMac72 I applied preyansh's changes to the latest codebase on my own fork, made some improvements, and added more telemetry.

I'm not sure if this is how this is how the maintainers want to deal with this PR, but they can take it or leave it. I opened a PR for reference, if nothing else.

#169

Wow thanks you're the best, will check it out, thank you so much for sharing!

Have you perhaps looked at any other feature enhancements as well, like handling uneven lists or manage active teams with icebreakers across different teams?

[markusd1984]

@corylk @preyansh98 for larger teams like @amannall mentioned with 250 users or more, i.e. I want to use it with just over 1k, would it be better to increase the # of users stored from 3 to something much higher to avoid more duplicated later on?

Are there any major concerns i.e. performance or was it just kept low in case the bot is used with very small teams?

[markusd1984]

@corylk just looked a bit more deeper and can see now that you incorporated the web.config setting MaxRecentPairUpsToPersistPerUser=20, so it's already increased to 20 from initial 3.

In my testing with 11 people I had experienced some users getting the same names already 2-3 rounds later on, too quick, but that might be because I also added the prevent unevenlist feature, which I guess won't work well together.

Any chance you can have a look if we could incorporate the below into the list of pairs please?

// If an odd number of users, randomly select then append one user to make an even list 
            if (users.Count % 2 != 0)
            {
                var random = new Random().Next(1, users.Count);
                users.Add(users[random]);
            }

perhaps adding a random user at https://github.com/corylk/microsoft-teams-apps-icebreaker/blob/77b1a51b6d2213b81685e30d0d1df47be4f04c7b/Source/Icebreaker/Services/MatchingService.cs#L263

I suppose though we should avoid users being paired with one self too, as I started seeing this happening after 2 weeks of testing. =) Maybe adding a condition if (pairUserOne == pairUserTwo) {break;} before checking if the pairs have been recently paired? Could that work?
https://github.com/corylk/microsoft-teams-apps-icebreaker/blob/77b1a51b6d2213b81685e30d0d1df47be4f04c7b/Source/Icebreaker/Services/MatchingService.cs#L235

UPDATE:
Managed to check the db and noticed that 3 out of 11 users had no RecentPairs stored at all, thus I suspect something went wrong.

I will try to test it without the uneven list feature (where one user gets randomly added to the userlist) but I'm wondering if it's something with the feature here. I couldn't find any TrackTrace message with "Error updating user info: ..." thus since all users received a pairing I would expect that all users should have run through the pairing process and recentPairUps should have been updated, but strangely I can only find 5 "Successfully updated user info for" messages when 6 pairings occurred.

[corylk]

@markusd1984

For the users without history, did you check that these 3 users are not opted out?

Users getting self-paired is without doubt a result of the code you posted. There might be other consequences of this as well as none of the pairing logic expects the set to contain duplicate users.

As for users getting paired 2-3 rounds later, this is quite possible with a set of only 11 users. The pairing logic randomizes the set of users, then iterates through them, pairing them with the next user that doesn't already have the same pair in their history. If no perfect pair is available, it simply picks the next user.

By the time you get to the last few users in the set, it's possible that no perfect pairs are available since most users are already matched. The likelihood of this happening is higher when the set is smaller and/or when MaxRecentPairUpsToPersistPerUser is too high.

This config value never needs to be higher than n-1 and I think you will get better results with an even smaller value. I'd aim for (n-1)/2.

[markusd1984]

@corylk yes they stayed opted in during the whole test period and in the screenshot you can see always 11 people in all columns (except in some more due to more multiple double pairing, probably due to the with the uneven feature).

I will revert on Mon/Tue with my test results, thanks for the feedback.

If no perfect pair is available, it simply picks the next user.

Does that not mean though for a set of 11 users that it should pair one user at least with all users over 10 rounds?
(while one would be missed each round and all would have to stay opted in)

Can you elaborate on n-1? Do you mean amount of users less one? For 1000 user what setting would you recommend? (1000/2=500 instead of the default 20 setting? )

[cocochrane]

What's the ETA for getting this feature merged? It's the top complaint from my team members using the app