Skip to content

PEDS-1486: (BE) - Optimization changes to improve delivery speed #115

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.

Sign up for GitHub

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

Open
kellcuc wants to merge 3 commits into
base: pcdc_dev
Choose a base branch
from
Open

PEDS-1486: (BE) - Optimization changes to improve delivery speed #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f21a7a9
Optimization changes to improve delivery speed
Nov 15, 2025
242b62e
Remove comment of logger
Nov 17, 2025
de939a1
update to match survival
Nov 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 29 additions & 26 deletions PcdcAnalysisTools/blueprint/routes/views/survival/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

DEFAULT_SURVIVAL_CONFIG = {"consortium": [], "excluded_variables": [], "result": {}}

# TODO: consider a simple cache keyed by (efs_flag, consortium, filters) to reuse guppy_data across repeated runs.

@kellcuc kellcuc Nov 15, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Note that we can add caching



@auth.authorize_for_analysis("access")
def get_config():
Expand All @@ -24,13 +26,20 @@ def get_config():

@auth.authorize_for_analysis("access")
def get_result():
# provides a Python dict built from the request JSON
args = utils.parse.parse_request_json()
config = capp.config.get("SURVIVAL", DEFAULT_SURVIVAL_CONFIG)

# TODO add json payload control
# TODO add check on payload nulls and stuff
# TODO add path in the config file or ENV variable
filter_sets = json.loads(json.dumps(args.get("filterSets")))

@kellcuc kellcuc Nov 15, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We were doing double work with json.loads(json.dumps(args.get("filterSets"))), and so we use what we already had in args args = utils.parse.parse_request_json() and parse that.

raw_filter_sets = args.get("filterSets") or []
if not isinstance(raw_filter_sets, list):
raise UserError("filterSets must be a list")

# clean copy of list
filter_sets = list(raw_filter_sets)

@kellcuc kellcuc Nov 15, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We go for a shallow vs deep copy of the list here, because i could not find that we needed inner dicts.


risktable_flag = config.get("result").get("risktable", False)
survival_flag = config.get("result").get("survival", False)
efs_flag = args.get('efsFlag', False)
Expand All @@ -47,6 +56,7 @@ def get_result():
capp.logger.warning(
"Unable to load or find the user, check your token"
)

capp.logger.info("SURVIVAL TOOL - " + json.dumps(log_obj))

survival_results = {}
Expand Down Expand Up @@ -80,6 +90,7 @@ def get_result():
AGE_AT_DISEASE_PHASE = "timings.age_at_disease_phase"
DISEASE_PHASE = "timings.disease_phase"


def fetch_data(config, filters, efs_flag):
status_str, status_var, time_var = (
(EVENT_FREE_STATUS_STR, EVENT_FREE_STATUS_VAR, EVENT_FREE_TIME_VAR)
Expand All @@ -94,23 +105,22 @@ def fetch_data(config, filters, efs_flag):
guppy_data = json.load(f)
else:
guppy_data = utils.guppy.downloadDataFromGuppy(
path=capp.config['GUPPY_API'] + "/download",
type="subject",
totalCount=100000,
fields=[status_var, time_var, DISEASE_PHASE, AGE_AT_DISEASE_PHASE],
filters=(
{"AND": [
{"IN": {"consortium": config.get('consortium')}},
filters
]}
if config.get('consortium')
else filters
),
sort=[],
accessibility="accessible",
config=capp.config
)

path=capp.config['GUPPY_API'] + "/download",
Comment thread
paulmurdoch19 marked this conversation as resolved.

@paulmurdoch19 paulmurdoch19 Nov 17, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the big reason why it takes so long is because we are using the /download endpoint which attempts to transmit 40000 dictionaries with 5 key pairs in prod for the all filter. I am not sure but if its possible to get the needed data but we could try and use the /graphql endpoint.

@kellcuc kellcuc Nov 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

graphql endpoint option, I will test (thanks for endpoint Paul).
https://github.com/chicagopcdc/guppy/blob/0751949be68b4936127e5478e757c8462a098a7a/src/server/server.js#L51

@kellcuc kellcuc Nov 18, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

using the path=capp.config['GUPPY_API'] + "/graphql", - 186 ms

{
    "3": {
        "count": {
            "fitted": 2,
            "total": 23
        },
        "name": "1. instruct",
        "risktable": [
            {
                "nrisk": 2,
                "time": 0
            },
            {
                "nrisk": 0,
                "time": 1
            }
        ],
        "survival": [
            {
                "prob": 1,
                "time": 0
            },
            {
                "prob": 0.5,
                "time": 0.03174112025878608
            },
            {
                "prob": 0,
                "time": 0.05307142996156295
            }
        ]
    }
}

Using the path=capp.config['GUPPY_API'] + "/download", - 161ms

{
  "3": {
    "count": {
      "fitted": 2,
      "total": 23
    },
    "name": "1. instruct",
    "risktable": [
      {
        "nrisk": 2,
        "time": 0
      },
      {
        "nrisk": 0,
        "time": 1
      }
    ],
    "survival": [
      {
        "prob": 1,
        "time": 0
      },
      {
        "prob": 0.5,
        "time": 0.03174112025878608
      },
      {
        "prob": 0,
        "time": 0.05307142996156295
      }
    ]
  }
}

@paulmurdoch19 paulmurdoch19 Dec 12, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

that's going to change at scale in prod there's 40k dictionaries that have to be downloaded in the worst case and that's what's causing it to take 15 sec. When a user selects a filter-set to use it checks to see if the filter-set is in scope using this endpoint /analysis/tools/stats/consortiums which uses the guppy download endpoint. Since we're only finding the consortium counts we can have the /analysis/tools/stats/consortiums use the graphql endpoint instead similar to how the summary page displays the counts.

@grugna grugna Dec 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the display counts is actually reading from the same download endpoint and cache the result since they are static between data release.

@paulmurdoch19 paulmurdoch19 Dec 24, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yes not the display counts the consortium count in the charts portion of the explorer page.
Screenshot 2025-12-23 at 5 23 43 PM

Comment thread
paulmurdoch19 marked this conversation as resolved.
type="subject",
totalCount=100000,
fields=[status_var, time_var, DISEASE_PHASE, AGE_AT_DISEASE_PHASE],
filters=(
{"AND": [
{"IN": {"consortium": config.get('consortium')}},
filters
]}
if config.get('consortium')
else filters
),
sort=[],
accessibility="accessible",
config=capp.config
)

node, age_at_disease_phase = AGE_AT_DISEASE_PHASE.split('.')
node, disease_phase = DISEASE_PHASE.split('.')
Expand Down Expand Up @@ -160,7 +170,6 @@ def fetch_data(config, filters, efs_flag):
raise NotFoundError("The cohort selected has no {} and/or no {}. The curve can't be built without these necessary data points.".format(
EVENT_FREE_STATUS_VAR if efs_flag else OVERALL_STATUS_VAR, EVENT_FREE_TIME_VAR if efs_flag else OVERALL_TIME_VAR))


return (
pd.DataFrame.from_records(guppy_data)
.assign(
Expand Down Expand Up @@ -208,7 +217,7 @@ def get_survival_result(data, risktable_flag, survival_flag):
# data_kmf['time'] = data_kmf['time'].astype(float)

# print(result)
data_kmf.info()
# data_kmf.info()
Comment thread
paulmurdoch19 marked this conversation as resolved.
# print(data_kmf)

if result["count"]["fitted"] == 0:
Expand Down Expand Up @@ -292,9 +301,3 @@ def check_allowed_filter(config, filter_set):
if value in user_filter_str:
raise UserError("One or more filters selected contains a variable that is not allowed. List of variable that are not allowed: {}".format(excluded_variables))
return True






36 changes: 21 additions & 15 deletions PcdcAnalysisTools/blueprint/routes/views/tableOne/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@
logger = get_logger(logger_name=__name__, log_level="info")

DEFAULT_TABLE_ONE_CONFIG = {"consortium": [], "excluded_variables": [], "enabled": True}


@auth.authorize_for_analysis("access")
def get_config():
config = capp.config.get("TABLE_ONE", DEFAULT_TABLE_ONE_CONFIG)
#change the name of the key from excluded_variables to excludedVariables
# change the name of the key from excluded_variables to excludedVariables
config = dict(config) # make a shallow copy so we don't modify the original
if "excluded_variables" in config:
config["excludedVariables"] = config.pop("excluded_variables")
Expand All @@ -28,8 +30,8 @@ def get_config():

# TODO - this is not used anywhere so I am not sure?? maybe it is the format expected for the `covariates` arg in the request
covarname = {
"sex":"sex",
"race":"race",
"sex": "sex",
"race": "race",
"survival_characteristics.lkss":"lkss"
}

Expand Down Expand Up @@ -96,9 +98,14 @@ def _check_user_input(args):

config = capp.config.get("TABLE_ONE", DEFAULT_TABLE_ONE_CONFIG)

filter_sets = args.get("filterSets")
raw_filter_sets = args.get("filterSets") or []
Comment thread
paulmurdoch19 marked this conversation as resolved.
if not isinstance(raw_filter_sets, list):
raise UserError("filterSets must be a list")

# clean copy of list
filter_sets = list(raw_filter_sets)
Comment thread
paulmurdoch19 marked this conversation as resolved.
covariates = args.get("covariates")
#for first version only allow one filter set
# for first version only allow one filter set
if not filter_sets:
raise UserError("You must submit a filter set")
if len(filter_sets) != 1:
Expand Down Expand Up @@ -150,8 +157,6 @@ def _check_user_input(args):
except KeyError as e:
raise UserError(f"Missing required field in filter set: {e}")



# TODO I assume we don't need a filter for all data aside from {} We will need to support consortiums limitations like for the survival.
# I guess this is the reason of this allFilter variable?
# we will also in addition to this need to check if the selected filters is allowed, like in the survival
Expand All @@ -170,7 +175,7 @@ def _check_user_input(args):


def _find_inverse_user_df(total_filter_set_df, user_filter_set_df):
### compare the two pandas dataframes and create a new dataframe of the rows from total_filter_set_df whose values in subject_submitter_id are not in user_filter_set_df
# compare the two pandas dataframes and create a new dataframe of the rows from total_filter_set_df whose values in subject_submitter_id are not in user_filter_set_df
if "subject_submitter_id" not in total_filter_set_df.columns or "subject_submitter_id" not in user_filter_set_df.columns:
raise InternalError("Both dataframes must contain the 'subject_submitter_id' column to find the inverse.")
# Find the subject_submitter_id values in user_filter_set_df
Expand All @@ -181,9 +186,10 @@ def _find_inverse_user_df(total_filter_set_df, user_filter_set_df):
inverse_df.reset_index(drop=True, inplace=True)
return inverse_df


def _fetch_data(filters, fields):
guppy_data = utils.guppy.downloadDataFromGuppy(
path=capp.config.get("GUPPY_API") + "/download",
path=capp.config['GUPPY_API'] + "/download",
Comment thread
paulmurdoch19 marked this conversation as resolved.
type="subject",
totalCount=100000,
fields=fields, # TODO - check it is a list
Expand All @@ -193,8 +199,8 @@ def _fetch_data(filters, fields):
config=capp.config
)


return(guppy_data)
return (guppy_data)


def _get_table_result(col_2_df, col_3_df, covariates, config):
true_number = len(col_2_df)
Expand All @@ -208,7 +214,7 @@ def _get_table_result(col_2_df, col_3_df, covariates, config):

covariate_return_value = {"name": name}
keys = []
#calcuate the total number of row values which are empty / data is missing for this column covariate['label']
# calcuate the total number of row values which are empty / data is missing for this column covariate['label']
if covariate['label'] not in col_2_df.columns:
raise InternalError(f"Covariate label '{covariate['label']}' not found in the data.")

Expand All @@ -221,9 +227,9 @@ def _get_table_result(col_2_df, col_3_df, covariates, config):
covariate_return_value["missingFromTotalCount"] = int(missing_count_from_total)

if covariate["type"] == "continuous":
#need to figure out how to find unit in data-portal
#removed unit for now/int(covariate['unit']
#this is rounding
# need to figure out how to find unit in data-portal
# removed unit for now/int(covariate['unit']
# this is rounding
true_mean = format(col_2_df[covariate['label']].mean(), '.1f' )
total_mean = format(col_3_df[covariate['label']].mean(), '.1f' )
covariate_return_value["mean"] = {f"true": true_mean, "total": total_mean}
Expand Down