Skip to content
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

feat: add score status recalculation to recalc.py #573

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

minisbett
Copy link
Contributor

@minisbett minisbett commented Feb 11, 2024

Describe your changes

Added a third re-calculation that re-evaluates the status of all non-failed scores (status>0)

The new re-calculation is important to perform when PP is being updated, but might also be desired to run isolated from PP re-calculation.
For all (map_md5, userid, mode) pairs it'll find the best score of the user (which is determined by PP in bpy) and sets the status to 2. The status of all other non-failed scores on that map are set to 1.

Discussing necessary before merge
The re-calculation of statuses is necessary since after the update of PP a score which was previously considered the best score by a user on a map might no longer be the best and replaced by a different, simply submitted (status=1) score.
In order to account for this, we would need to not only re-calculate all status=2 scores, but also all status=1 scores. This would introduce a significantly higher time consumption for running the script.

For my 12-day old server, I currently have 54,346 scores with status=2 and 73,475 scores with status>=1, out of 305,761 total scores. That's an increase of around 35%, but will increase as time goes on as users will start playing the same maps more causing more submitted, but not best scores.

Checklist

  • I've manually tested my code

@cmyui
Copy link
Member

cmyui commented Feb 12, 2024

  1. The description attribute from argparse is now called help and has been renamed.

Could you link documentation for this? Py docs still show description: https://docs.python.org/3.11/library/argparse.html

I find it kinda unlikely python would make a breaking change for this, but perhaps?

@minisbett
Copy link
Contributor Author

minisbett commented Feb 12, 2024

  1. The description attribute from argparse is now called help and has been renamed.

Could you link documentation for this? Py docs still show description: https://docs.python.org/3.11/library/argparse.html

I find it kinda unlikely python would make a breaking change for this, but perhaps?

I was looking at https://docs.python.org/3/library/argparse.html, which seems to have been correct for me.
image
Also this python version is what I used to run, right inside the banchopy docker container.

@NiceAesth
Copy link
Contributor

NiceAesth commented Feb 13, 2024

cherrypicked the fixes from this PR into #587 #588

please keep pr goals singular and separate them when necessary

@NiceAesth NiceAesth changed the title feat.: Fix description attribute error & add score status recalculation to recalc.py feat: add score status recalculation to recalc.py Feb 13, 2024
@GamebP
Copy link

GamebP commented Mar 5, 2024

merging this can make fake data work. Am i correct?

@minisbett
Copy link
Contributor Author

merging this can make fake data work. Am i correct?

Fake data works without this. What you are trying to do is just not what you use fake data for.

Copy link
Contributor

@tsunyoku tsunyoku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appreciate this is a super late review, but only one general issue i see with this. would be good if this could be addressed, i was reminded about this PR following the akatsuki-pp-py bump 😅

Comment on lines +237 to +246
await ctx.database.execute(
"UPDATE scores SET status = 1 "
"WHERE map_md5 = :map_md5 AND userid = :userid AND mode = :mode AND status > 0",
pair,
)

await ctx.database.execute(
"UPDATE scores SET status = 2 WHERE id = :id",
{"id": best["id"]},
)
Copy link
Contributor

@tsunyoku tsunyoku Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally this runs in a transaction, if an API request comes in inbetween these 2 queries or something then they're going to have no best on the map.

additionally, for no reason other than being picky, would be good to add a AND id != :best_id on the first query just so that it doesn't get unnecessarily touched until the following query

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working developer tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants