Skip to content

Refactor callbacks in preparation for filtering#571

Merged
ElliottKasoar merged 18 commits into
ddmms:mainfrom
ElliottKasoar:update-callbacks
May 28, 2026
Merged

Refactor callbacks in preparation for filtering#571
ElliottKasoar merged 18 commits into
ddmms:mainfrom
ElliottKasoar:update-callbacks

Conversation

@ElliottKasoar
Copy link
Copy Markdown
Collaborator

Pre-review checklist for PR author

PR author must check the checkboxes below when creating the PR.

Summary

Continuation of #559.

In particular, this combines callbacks that recalculate summary tables to take in all benchmark states, as otherwise the the order of updates will impact the summary values calculated due to out-of-date states.

This has the unfortunate side-effect of making individual benchmark changes much heavier, so I've attempted to optimise score calculations a little.

This also fixes some issues to do with syncing stores/values, and allowing scores to be None/NaNs.

More work would definitely be welcome here with respect to optimisation in particular, as responsiveness may be an issue. It may also be good to prevent changes to inputs boxes while changes are propagating, if possible?

@ElliottKasoar ElliottKasoar requested a review from joehart2001 May 27, 2026 01:00
@ElliottKasoar ElliottKasoar added the enhancement New feature or request label May 27, 2026
@ElliottKasoar ElliottKasoar changed the title Refactor callbacks Refactor callbacks in preparation for filtering May 27, 2026
Comment thread ml_peg/analysis/utils/utils.py
Comment thread ml_peg/analysis/utils/utils.py
@joehart2001
Copy link
Copy Markdown
Collaborator

joehart2001 commented May 27, 2026

  • changes thresholds doesnt seem to update the category tables. i think this is due to duplicate stores, as you set them globally but then we set them again later (line 856 in build_components.py). commenting out this block seemed to fix it for me. along with id=f"{table.id}-computed-store" we also store id=f"{table.id}-raw-data-store" and id=f"{table.id}-raw-tooltip-store" so we need to think about those.
    • maybe removing this duplication will help with the speed
  • the standalone benchmark apps wont work properly anymore, but i dont think this is an issue. maybe we can remove logic related to those tbh

performance:

  • check out dash Patch to only update information which changes

  • i think we should probably add some form of caching down the line

  • online says we may be able to store things server side to help, but i dont know much about this

  • idea: i think this makes having global resets for all weights and all thresholds much easier, i'll create an issue

@ElliottKasoar
Copy link
Copy Markdown
Collaborator Author

  • changes thresholds doesnt seem to update the category tables. i think this is due to duplicate stores, as you set them globally but then we set them again later (line 856 in build_components.py). commenting out this block seemed to fix it for me. along with id=f"{table.id}-computed-store" we also store id=f"{table.id}-raw-data-store" and id=f"{table.id}-raw-tooltip-store" so we need to think about those.

    • maybe removing this duplication will help with the speed
  • the standalone benchmark apps wont work properly anymore, but i dont think this is an issue. maybe we can remove logic related to those tbh

performance:

  • check out dash Patch to only update information which changes
  • i think we should probably add some form of caching down the line
  • online says we may be able to store things server side to help, but i dont know much about this
  • idea: i think this makes having global resets for all weights and all thresholds much easier, i'll create an issue

Thanks for taking a look!

Changing the thresholds seems to work for me. Are you sure it's finished fully loading?

I'm not completely sure whether the duplicate stores should be a problem, but you're right that they're not necessary - I missed copying those deletions from #559, so added that back in now. The tooltip store is still page-specific though, we don't need those globally.

Yes, I did realise that the standalone apps won't work. We can add an option to use ml_peg app with specific tests, which is slightly heavier than we need as it'll build the summary tables, but better than nothing? We could also make it work by calling the right functions if we really wanted to for each app.

I have come across patches/partial updates, but it's somewhat unclear how much benefit we'd get. Would you be able to look into it a bit?

The change in performance is because we're going from one benchmark table update -> update one column of category summary to one benchmark table update -> recalculate full category summary table.

This is obviously overkill when you only change one table, but when all the tables change, you risk out of sync information when you write out the new category summary data.

It's definitely possible that partial updates may help with this, but it's not obvious from the docs that it works on quite such a fine level.

@joehart2001
Copy link
Copy Markdown
Collaborator

  • changes thresholds doesnt seem to update the category tables. i think this is due to duplicate stores, as you set them globally but then we set them again later (line 856 in build_components.py). commenting out this block seemed to fix it for me. along with id=f"{table.id}-computed-store" we also store id=f"{table.id}-raw-data-store" and id=f"{table.id}-raw-tooltip-store" so we need to think about those.

    • maybe removing this duplication will help with the speed
  • the standalone benchmark apps wont work properly anymore, but i dont think this is an issue. maybe we can remove logic related to those tbh

performance:

  • check out dash Patch to only update information which changes
  • i think we should probably add some form of caching down the line
  • online says we may be able to store things server side to help, but i dont know much about this
  • idea: i think this makes having global resets for all weights and all thresholds much easier, i'll create an issue

Thanks for taking a look!

Changing the thresholds seems to work for me. Are you sure it's finished fully loading?

I'm not completely sure whether the duplicate stores should be a problem, but you're right that they're not necessary - I missed copying those deletions from #559, so added that back in now. The tooltip store is still page-specific though, we don't need those globally.

Yes, I did realise that the standalone apps won't work. We can add an option to use ml_peg app with specific tests, which is slightly heavier than we need as it'll build the summary tables, but better than nothing? We could also make it work by calling the right functions if we really wanted to for each app.

I have come across patches/partial updates, but it's somewhat unclear how much benefit we'd get. Would you be able to look into it a bit?

The change in performance is because we're going from one benchmark table update -> update one column of category summary to one benchmark table update -> recalculate full category summary table.

This is obviously overkill when you only change one table, but when all the tables change, you risk out of sync information when you write out the new category summary data.

It's definitely possible that partial updates may help with this, but it's not obvious from the docs that it works on quite such a fine level.

i renamed my app/data directory to start from a clean one and reran analysis for some categories and the thresholds update the benchmark tables but dont propagate to the category tables until i remove the duplicate store i mentioned.

for the other stuff, probably a future pr?

@joehart2001
Copy link
Copy Markdown
Collaborator

now the category table update seems good, but its not propagating to the summary table

@ElliottKasoar ElliottKasoar merged commit f0e7f15 into ddmms:main May 28, 2026
7 checks passed
@ElliottKasoar ElliottKasoar deleted the update-callbacks branch May 28, 2026 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants