Refactor of inputs app#624
Open
tomjemmett wants to merge 27 commits into
Open
Conversation
80061da to
fc134f6
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the Shiny “inputs” app to reduce startup latency in dev, centralize lookup loading, and simplify module wiring by removing legacy caching/reactives and moving some data/schema loading to singleton helpers.
Changes:
- Adds ADLS download helpers (
download_provider_data(),get_all_data_files()) and switches runtime loading to local parquet reads viaload_provider_data(). - Introduces a singleton
get_lookups()environment and updates modules to consume lookups directly (instead of passing reactives around). - Refactors schema handling to a singleton (
get_params_schema()), and removes legacy caching/startup mechanisms (e.g.,create_data_cache(),start()reactive, waiting list tab).
Reviewed changes
Copilot reviewed 27 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| man/load_provider_data.Rd | Updates generated docs for load_provider_data() signature. |
| man/get_all_data_files.Rd | Adds generated docs for get_all_data_files(). |
| man/download_provider_data.Rd | Adds generated docs for download_provider_data(). |
| inst/golem-config.yml | Removes redundant slider config values from mitigator config. |
| inst/app/reference/waiting_list_params.csv | Adds reference CSV for waiting list multipliers. |
| inst/app/reference/rtt_specialties.csv | Adds RTT specialties reference data. |
| inst/app/reference/providers.csv | Adds providers lookup reference data. |
| inst/app/reference/procedures.csv | Adds procedures lookup reference data. |
| inst/app/reference/peers.csv | Adds peers lookup reference data. |
| inst/app/reference/nee_table.csv | Adds NEE lookup reference data. |
| inst/app/reference/ndg_variants.json | Adds NDG variants lookup reference data. |
| inst/app/reference/mitigator-codes.csv | Adds mitigator codes lookup reference data. |
| R/run_app.R | Ensures data directory exists at app startup; triggers initial download. |
| R/mod_waiting_list_imbalances_utils.R | Switches RTT specialties lookup access to get_lookups(). |
| R/mod_waiting_list_imbalances_server.R | Switches waiting list multipliers to get_lookups() output. |
| R/mod_run_model_server.R | Removes schema text parameter and uses schema singleton. |
| R/mod_run_model_remove_invalid_mitigators.R | Fixes NULL errors edge case by early-return on valid JSON. |
| R/mod_run_model_fix_params.R | Updates calls to new schema/migitator-cleanup signatures. |
| R/mod_non_demographic_adjustment_server.R | Switches NDG variants loading to get_lookups(). |
| R/mod_mitigators_summary_server.R | Switches mitigator lookup usage to get_lookups() and relies on filtered reactive data inputs. |
| R/mod_mitigators_server.R | Refactors module init, replaces passed-in lookups with get_lookups(), and reorganizes observers/outputs. |
| R/mod_inequalities_server.R | Refactors to load inequalities data internally via get_inequalities_data(). |
| R/mod_home_server.R | Simplifies signature (providers no longer passed in). |
| R/mod_expat_repat_ui.R | Adds spinners around plots/map outputs. |
| R/mod_expat_repat_server.R | Refactors to load data internally and uses lookup singleton for providers/specialties/boundaries. |
| R/mod_baseline_adjustment_ui.R | Switches RTT specialties loading to get_lookups(). |
| R/mod_baseline_adjustment_server.R | Refactors to load baseline data internally and uses lookup singleton. |
| R/fct_utils_lookups.R | Introduces singleton lookup loader for CSV/JSON/SF reference data. |
| R/fct_reduce_values.R | Removes reduce_values() helper. |
| R/fct_load_provider_data.R | Adds in-memory cache and helpers for reading provider parquet datasets. |
| R/fct_create_data_cache.R | Removes disk cache helper that configured shinyOptions(cache=...). |
| R/fct_azure_storage.R | Replaces load_provider_data() ADLS read with download helpers and adds bulk download function. |
| R/app_ui.R | Removes waiting list imbalances tab from UI. |
| R/app_server.R | Removes legacy caching/start reactive; rewires module initialization and data loading to new helpers. |
| R/ZZZ.R | Adds schema download via httr2 and a cached schema loader; removes old rtt_specialties() helper. |
| .gitignore | Ignores inst/app/data/ (downloaded parquet + schema). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Thanks Tom! As discussed: we'll release v5.0 first, then attend to this before it gets stale. |
4aaece3 to
5a81404
Compare
removes code that is no longer needed. the config$params_items is now all just values and no longer functions that need to be evaluated switches from a function to get the default interval to hardcoded values - the function ignored the argument and always returned c(0.95, 1.0)
…ting of number which is no longer needed
use once=TRUE instead of init$destroy
prevents lookups having to be loaded multiple times for each different connection to the server
* no longer tries to download files in the shiny app * adds dev/get_data_files.R to download the files to inst/app/data * removes caching as no longer needed * moves logic for reshaping the required data from the app_server function to their own functions in fct_load_provider_data.R
5a81404 to
c093082
Compare
c093082 to
f958388
Compare
reverted to what we had before, except without the handling of function calls
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Various refactors to improve the codebase and remove some of the historic code that doesn't serve a purpose any longer, and to make some strides to improve the apps responsiveness.
Remove the
start()reactiveNo longer serving any purpose, removed this in favour of a
shiny::req(params$dataset)instead.Altered handling of data files
Previously, we loaded all of the data files from azure inside of the server function, caching the reactive results. But, caching wasn't enabled for dev workflows, so our dev workflow was always slowed down by needing to re-download files.
Instead, we now have a function
get_all_data_files()which is called inrun_appif the folderinst/app/datadoes not exist.This has necessitated renaming the existing data folder to
inst/app/reference.Used the same pattern for the json schema, this is downloaded by the
get_all_data_files()function.If the data files change, for dev purposes you can simply re-run the function. It will re-download and overwrite all the files.
Move all lookup files into a lookups environment
Rather than loading the lookups into reactives, then evaluating these and passing them to the modules, we now have a centralised lookups singleton.
All of the cases where we use the lookups are moved into the modules themselves, making the main server module much cleaner.
Removed redundant config values
Some options in the
golem-config.ymlno longer server any purpose. Removed these.Removed caching code
No longer need to rely on shiny for caching, we persist the data on initial load. During shiny deployments, the inst/app/data folder will not be present. It will be created the first time the app is run after any redeployment.
Improvements to schema validation
There was a potential issue in
mod_run_model_remove_invalid_mitigators- if the json provided was valid, then theerrorsattribute can beNULL, so the subsequentdplyrcode could error out.Loading the schema can take some time, moved this into a singleton pattern so the schema only needs to be loaded once.
Switch from
download.fileto usinghttr2to load the params file - this gets rid of some janky code to append missing final newlines