Fix scores set with unranked mods showing as processing indefinitely (again) #11151

bdach · 2024-04-09T10:02:42Z

Closes Scores set with mods which don't support pp show as "This score is still being calculated" #10911 once more.
Regressed with Stop marking scores as ranked = 0 for pp only purposes osu-queue-score-statistics#227.

The actual fix is the inclusion of and check for existence of the score_process_history row as it is the most reliable indicator of processing having concluded. (cc @peppy as you expressed being hesitant about using it)

This also includes a drive-by inclusion of a preserve === false check that won't really do anything right now, but will if / once ppy/osu-queue-score-statistics#141 is resolved.

The goal here is to only show pp values on scores that contribute to the user's total. The query determining can be found here.

…(again) Closes ppy#10911 once more. The actual fix is the inclusion of and check for existence of the `score_process_history` row as it is the most reliable indicator of processing having concluded. This also includes a drive-by inclusion of a `preserve === false` check that won't really do anything right now, but will if / once ppy/osu-queue-score-statistics#141 is resolved. The goal here is to only show pp values on scores that contribute to the user's total. The query determining can be found here: https://github.com/ppy/osu-queue-score-statistics/blob/f65dec335a914ee171d69981719d6ccb1d3f0ebe/osu.Server.Queues.ScoreStatisticsProcessor/Processors/UserTotalPerformanceProcessor.cs#L66-L76

notbakaneko · 2024-04-09T11:18:02Z

At this point I feel like we're better off checking with mods award pp or just storing when a score isn't supposed to have any pp.
Is every score guaranteed to have a corresponding score_process_history row? Does score_process_history get truncated?

bdach · 2024-04-09T12:13:17Z

At this point I feel like we're better off checking with mods award pp

That feels like a bad ending IMO. The logic isn't simple and is (hopefully?) going to be subject to frequent changes as we bring pp to more mods over time.

or just storing when a score isn't supposed to have any pp.

I guess I could stomach this although that's yet another flag to the collection in the table. Maybe fine (although ranked has been confusing everyone multiple times over now, but I can't really bring myself to harp on about this any longer - I think I did what I could in that respect in documenting what it does).

Is every score guaranteed to have a corresponding score_process_history row? Does score_process_history get truncated?

As far as I'm aware, yes. See: https://github.com/ppy/osu-queue-score-statistics/blob/f65dec335a914ee171d69981719d6ccb1d3f0ebe/osu.Server.Queues.ScoreStatisticsProcessor/ScoreStatisticsQueueProcessor.cs#L270-L280.

I dunno what "truncation" would mean in this context.

notbakaneko · 2024-04-10T06:03:03Z

I dunno what "truncation" would mean in this context.

I mean if old rows ever get dropped or deleted from the table.

bdach · 2024-04-10T06:08:37Z

I mean if old rows ever get dropped or deleted from the table.

Unless the score itself is deleted, currently I don't think so. Even if the score is pushed for reprocessing, the row remains, it'll just get updated after reprocessing is done.

peppy · 2024-04-10T10:40:51Z

As a heads up, I was considering removing the process history table as it's not used for much at all. So there's no guarantee it will exist forever (I'd rather avoid new uses of it >.>).

bdach · 2024-04-10T10:44:59Z

Well that's fine and all, but that leaves me without a clear alternative path. Some were already proposed above, but will require either further implementation (reimplementing ranked mod logic web-side) or a db schema change and back-migration (new db column indicating if score gives pp) somehow.

peppy · 2024-04-10T11:04:43Z

Yep understood. I'm gonna have to take a step back and decide how complicated we want/need to make this whole system, because it is feeling increasingly unmaintainable (and I am sure I'm not alone in this train of thought).

peppy · 2024-04-11T07:08:06Z

app/Transformers/ScoreTransformer.php

@@ -102,6 +102,7 @@ public function transformSolo(MultiplayerScoreLink|ScoreModel|SoloScore $score)
        if ($score instanceof SoloScore) {
            $extraAttributes['ranked'] = $score->ranked;
            $extraAttributes['preserve'] = $score->preserve;
+            $extraAttributes['processed'] = $score->processHistory()->first() !== null;


I'm a bit concerned about this being in the transformer here – is there potential that it adds an overhead (+1 query?) in places we don't want it to?

Hmm well it probably does but I'm not super sure how to do better... It does kinda need to be in the transformer because we'll want to use this in client to mirror this logic too.

I don't think anything other than a new column in scores solves this.

processHistory definitely needs to be eager loaded in all the collections it's used in at the very least.

Well this is where I probably expose myself for a fraud but... do you have any good ways of determining where that should be added? It's a bit impossible to do a "find all usages" as I would with a more statically-typed language (unless I am using inadequate tools, which I might be, since my tools for php are just vscode right now).

I see MULTIPLAYER_BASE_PRELOAD in ScoreTransformer, and USER_PROFILE_INCLUDES_PRELOAD next to it, and that they get fed into with() in queries, but those appear to only be specific usage contexts too, which is probably not enough... I also see $defaultIncludes but I don't think that is going to help me here any is it?

$defaultIncludes is just the transformer defaults

peppy · 2024-04-11T07:08:56Z

@bdach the table is actually very small for process history so i'm fine with using it here for now. the main reason for potentially doing away with it was database overheads, but that is a non-issue for now.

peppy · 2024-04-15T03:31:17Z

Let's go ahead with this solution for now. Will defer for review.

notbakaneko

the following places need 'processHistory' added into the eager loads:

osu-web/app/Transformers/ScoreTransformer.php

Line 35 in 97f41e3

const USER_PROFILE_INCLUDES_PRELOAD = [

osu-web/app/Http/Controllers/BeatmapsController.php

Line 78 in 97f41e3

$scores = $esFetch->all()->loadMissing(['beatmap', 'user.country']);

osu-web/app/Http/Controllers/BeatmapsController.php

Line 497 in 97f41e3

$scores = (new ScoreSearch($params))->records();

->records()->loadMissing('processHistory');

'scoreLink.score.processHistory' also needs to be added after this

osu-web/app/Transformers/ScoreTransformer.php

Line 26 in 97f41e3

'scoreLink.score',

notbakaneko · 2024-04-16T13:06:03Z

app/Transformers/ScoreTransformer.php

@@ -102,6 +102,7 @@ public function transformSolo(MultiplayerScoreLink|ScoreModel|SoloScore $score)
        if ($score instanceof SoloScore) {
            $extraAttributes['ranked'] = $score->ranked;
            $extraAttributes['preserve'] = $score->preserve;
+            $extraAttributes['processed'] = $score->processHistory()->first() !== null;


Should be $score->processHistory !== null instead.

I'm not sure what I'm doing wrong but I swear I tried doing that first and it didn't work properly. In artisan tinker it does this:

> SoloScore::find(7)->processHistory() = Illuminate\Database\Eloquent\Relations\HasOne {#8056} > SoloScore::find(7)->processHistory()->first() = null > SoloScore::find(7)->processHistory() !== null = true > SoloScore::find(7)->processHistory()->first() !== null = false

on a scores row without associated score_process_history.

And that's even after consulting laravel docs which say that this should just work as you say. Did I mess something up here?

I changed to ->exists() in e25f033 since it seems better but yeah I dunno. Can't seem to get it working without some method call at the end.

oh, whoops, forgot to mention you have to add processHistory here for the accessor to work (because we have magic going on here to make it faster):

osu-web/app/Models/Solo/Score.php

Line 242 in 97f41e3

'performance',

processHistory() returns the relation so any eager loads with be ignored, processHistory will use the existing value if already loaded.

Yeah okay, that helped, thanks 👍

bdach · 2024-04-16T16:26:24Z

I've added the requested preloads I think but not entirely sure how to test. As far as I can tell the related views load fine...?

Imported legacy scores are not pushed for processing to the new score processor, so they will never actually receive a `score_process_history` row. Thankfully we can just consider all legacy scores processed since the legacy processing is synchronous (if a `scores` row exists for a legacy score, the score is already fully processed), but I'm not super sure what to make of this yet another complication...

bdach · 2024-04-17T09:40:13Z

app/Transformers/ScoreTransformer.php

@@ -102,6 +104,7 @@ public function transformSolo(MultiplayerScoreLink|ScoreModel|SoloScore $score)
        if ($score instanceof SoloScore) {
            $extraAttributes['ranked'] = $score->ranked;
            $extraAttributes['preserve'] = $score->preserve;
+            $extraAttributes['processed'] = $score->legacy_score_id !== null || $score->processHistory !== null;


Uhhh... So earlier I said:

Is every score guaranteed to have a corresponding score_process_history row? Does score_process_history get truncated?

As far as I'm aware, yes.

So as it turns out, I lied here (or if you wanna be charitable, "forgot"). It is not always the case that a score_process_history row will exist for a score that is processed.

The exception is scores that have been initially manually batch-imported from osu_scores_high_* tables. All legacy scores generally are fully processed regardless of the existence of that score_process_history row, because first osu-web-10 processes them synchronously, and then the score watchers import them without doing any further processing relevant to the score - so at the point the scores row exists for a legacy score, it is already fully processed.

I can't exactly double-check production to confirm this but I realised this when testing on my local database wherein I had a local score import from seeded osu_scores_high_* rows into scores.

What this says about this particular PR, and also the complexity in and around the new score table, I'm not entirely sure, but I do acknowledge that it's pretty bad, but also don't really have anything constructive to offer that isn't like a week spent on revisiting the entirety of the table structure again and attempting to somehow beat it into shape better...

notbakaneko · 2024-04-22T07:59:18Z

app/Transformers/ScoreTransformer.php

@@ -37,6 +38,7 @@ class ScoreTransformer extends TransformerAbstract
        'beatmap.beatmapset',
        // it's for user profile so the user is already available
        // 'user',
+        'processHistory',


Should be above the comment.

bdach added 2 commits April 9, 2024 14:14

Appease the linter

774ab79

Appease the other linter

97f41e3

peppy self-requested a review April 10, 2024 06:20

bdach mentioned this pull request Apr 10, 2024

Exclude scores without pp from user best performance #11153

Merged

peppy reviewed Apr 11, 2024

View reviewed changes

peppy requested a review from notbakaneko April 15, 2024 03:31

notbakaneko requested changes Apr 16, 2024

View reviewed changes

bdach added 3 commits April 16, 2024 16:34

Merge branch 'master' into fix-pp-display-yet-again

fdfd5d1

Switch null check to ->exists() instead

e25f033

Add requested eager loads

0cb8b4f

notbakaneko added area:user-profile area:beatmap-info labels Apr 17, 2024

bdach added 2 commits April 17, 2024 11:23

Fix direct access to processHistory one-to-one relation not working

754463c

bdach commented Apr 17, 2024

View reviewed changes

notbakaneko reviewed Apr 22, 2024

View reviewed changes

bdach added 2 commits April 22, 2024 11:32

Merge branch 'master' into fix-pp-display-yet-again

8239e94

Move the thing

3990207

notbakaneko approved these changes Apr 22, 2024

View reviewed changes

notbakaneko merged commit 581963f into ppy:master Apr 22, 2024
3 checks passed

bdach deleted the fix-pp-display-yet-again branch April 22, 2024 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scores set with unranked mods showing as processing indefinitely (again) #11151

Fix scores set with unranked mods showing as processing indefinitely (again) #11151

bdach commented Apr 9, 2024

notbakaneko commented Apr 9, 2024

bdach commented Apr 9, 2024 •

edited

notbakaneko commented Apr 10, 2024 •

edited

bdach commented Apr 10, 2024

peppy commented Apr 10, 2024

bdach commented Apr 10, 2024 •

edited

peppy commented Apr 10, 2024

peppy Apr 11, 2024

bdach Apr 11, 2024 •

edited

notbakaneko Apr 15, 2024

bdach Apr 15, 2024 •

edited

notbakaneko Apr 16, 2024

peppy commented Apr 11, 2024

peppy commented Apr 15, 2024

notbakaneko left a comment •

edited

notbakaneko Apr 16, 2024

bdach Apr 16, 2024 •

edited

bdach Apr 16, 2024

notbakaneko Apr 17, 2024 •

edited

bdach Apr 17, 2024

bdach commented Apr 16, 2024

bdach Apr 17, 2024 •

edited

notbakaneko Apr 22, 2024

Fix scores set with unranked mods showing as processing indefinitely (again) #11151

Fix scores set with unranked mods showing as processing indefinitely (again) #11151

Conversation

bdach commented Apr 9, 2024

notbakaneko commented Apr 9, 2024

bdach commented Apr 9, 2024 • edited

notbakaneko commented Apr 10, 2024 • edited

bdach commented Apr 10, 2024

peppy commented Apr 10, 2024

bdach commented Apr 10, 2024 • edited

peppy commented Apr 10, 2024

peppy Apr 11, 2024

Choose a reason for hiding this comment

bdach Apr 11, 2024 • edited

Choose a reason for hiding this comment

notbakaneko Apr 15, 2024

Choose a reason for hiding this comment

bdach Apr 15, 2024 • edited

Choose a reason for hiding this comment

notbakaneko Apr 16, 2024

Choose a reason for hiding this comment

peppy commented Apr 11, 2024

peppy commented Apr 15, 2024

notbakaneko left a comment • edited

Choose a reason for hiding this comment

notbakaneko Apr 16, 2024

Choose a reason for hiding this comment

bdach Apr 16, 2024 • edited

Choose a reason for hiding this comment

bdach Apr 16, 2024

Choose a reason for hiding this comment

notbakaneko Apr 17, 2024 • edited

Choose a reason for hiding this comment

bdach Apr 17, 2024

Choose a reason for hiding this comment

bdach commented Apr 16, 2024

bdach Apr 17, 2024 • edited

Choose a reason for hiding this comment

notbakaneko Apr 22, 2024

Choose a reason for hiding this comment

bdach commented Apr 9, 2024 •

edited

notbakaneko commented Apr 10, 2024 •

edited

bdach commented Apr 10, 2024 •

edited

bdach Apr 11, 2024 •

edited

bdach Apr 15, 2024 •

edited

notbakaneko left a comment •

edited

bdach Apr 16, 2024 •

edited

notbakaneko Apr 17, 2024 •

edited

bdach Apr 17, 2024 •

edited