fix: always select fallback if available #422
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
=======================================
Coverage 97.40% 97.40%
=======================================
Files 398 398
Lines 33575 33582 +7
=======================================
+ Hits 32703 32710 +7
Misses 872 872
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #422 +/- ##
=======================================
Coverage 97.40% 97.40%
=======================================
Files 398 398
Lines 33575 33582 +7
=======================================
+ Hits 32703 32710 +7
Misses 872 872
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
=======================================
Coverage 97.42% 97.42%
=======================================
Files 429 429
Lines 34266 34273 +7
=======================================
+ Hits 33385 33392 +7
Misses 881 881
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -38,14 +38,17 @@ def _get_installation_weight(installation: GithubAppInstallation) -> int: | |||
| return MAX_GITHUB_APP_SELECTION_WEIGHT | |||
| seconds_in_hour = 3600 | |||
| age_hours = (age.seconds // seconds_in_hour) + age.days * 24 | |||
| return age_hours + 2**age.days | |||
| # Prevent clock differences from making the weight negative | |||
| return max(1, age_hours + 2**age.days) | |||
|
|
|||
|
|
|||
| def _get_apps_from_weighted_selection( | |||
There was a problem hiding this comment.
I think it might be useful to add some function documentation around what it's returning because it looks like we're significantly changing the functionality. If I understand the code correctly, we're trying to get an ordered list of apps that's random based on the weighting (so that we don't always use the highest weighted one). The return is no longer just a "random subset of the apps" but an ordered list of apps (with some randomness).
There was a problem hiding this comment.
That is true... good callout here.
services/bots.py
Outdated
| # Obviously apps with a higher weight have a higher change of being selected as the main app (1st selection) | ||
| # But it's important that others are also selected so we can use them as fallbacks | ||
| apps_to_consider = [] | ||
| if keys: |
There was a problem hiding this comment.
Can we remove this if check here (i.e. the while loop condition should fail if the list is empty)?
There was a problem hiding this comment.
Sure, I'll refactor this
Because `random.choices` makes selections with replacement and we were making 3 selections only there was a chance that an app would vanish from the choices (from not being selected). This is bad because it hides this app as a possible fallbakc option. Particularly if the choice is marked currently as "rate limited" we would fail with exception instead of falling back in the other app. We want the fallback. These changes go around this "choice with selection" by removing the selected app from the population after each step. It's maybe slower (in practice the population will be small enough that this doesn't really matter), but makes sure that we always select all available apps. And still respectes the weighted selection.
Add docstrings to `_get_apps_from_weighted_selection` and refactor it to make the code easier to understand. And reduce code complexity.
giovanni-guidini
force-pushed
the
gio/fix-app-selection
branch
from
23bb5f8
to
6065c59
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #422 +/- ##
=======================================
Coverage 97.40% 97.40%
=======================================
Files 398 398
Lines 33575 33582 +7
=======================================
+ Hits 32703 32710 +7
Misses 872 872
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| or (not repository) | ||
| ) | ||
| ), | ||
| lambda obj: _can_use_this_app(obj, installation_name, repository), |
There was a problem hiding this comment.
This is much more readable now! Thanks!
| if installation_name != GITHUB_APP_INSTALLATION_DEFAULT_NAME: | ||
| # Add the default app as the last fallback if the owner is using a different app for the task | ||
| default_apps = filter( | ||
| lambda obj: obj.name == GITHUB_APP_INSTALLATION_DEFAULT_NAME, | ||
| lambda obj: _can_use_this_app( |
There was a problem hiding this comment.
Just double checking that this is OK. _can_use_this_app has extra logic that's not in the original functionality of this code, but those extra checks are required to be able to use the app anyways(?)
There was a problem hiding this comment.
I actually noticed when refactoring that the repo verification was missing from this filter. I think it is necessary to have that too, because if the repo is not covered we can't use the app.
Of course, considering the default installation, we expect all repos (that Codecov has access to) to be covered, but it's better for the logic to be consistent.
There was a problem hiding this comment.
About the is_configured check is a similar story. We generaly expect the default app to be configured (otherwise it wouldn't have that name actually), but if it's not configured we shouldn't use it.
Because
random.choicesmakes selections with replacement and we were making 3 selections onlythere was a chance that an app would vanish from the choices (from not being selected).
This is bad because it hides this app as a possible fallbakc option.
Particularly if the choice is marked currently as "rate limited" we would fail with exception
instead of falling back in the other app. We want the fallback.
These changes go around this "choice with selection" by removing the selected app from the
population after each step. It's maybe slower (in practice the population will be small enough
that this doesn't really matter), but makes sure that we always select all available apps.
And still respectes the weighted selection.
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.