Skip to content

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

Open
kellcuc wants to merge 3 commits into
pcdc_devfrom
PEDS-1486
Open

PEDS-1486: (BE) - Optimization changes to improve delivery speed#115
kellcuc wants to merge 3 commits into
pcdc_devfrom
PEDS-1486

Conversation

@kellcuc

@kellcuc kellcuc commented Nov 15, 2025

Copy link
Copy Markdown

JIRA: https://pcdc.atlassian.net/browse/PEDS-1486

After changes
Screenshot 2025-11-15 at 12 45 45 AM

Before changes
Screenshot 2025-11-15 at 12 53 31 AM

Locally, the file changed in size, but we dropped 271ms which should be a savings in prod. One of the big items was the logging we were outputting, as well as a circle loop, that seemed over repetitive. An option for more speed, would be a caching layer, and we also look at 100000 at one time, maybe lower this, unless there is a reason for such a large pull?


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.

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

# 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")))

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.

raise UserError("filterSets must be a list")

# clean copy of list
filter_sets = list(raw_filter_sets)

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.

Comment thread PcdcAnalysisTools/blueprint/routes/views/survival/__init__.py Outdated
Comment thread PcdcAnalysisTools/blueprint/routes/views/survival/__init__.py
Comment thread PcdcAnalysisTools/blueprint/routes/views/survival/__init__.py
@kellcuc kellcuc changed the title PEDS-1486: BE - Optimization changes to improve delivery speed PEDS-1486: (BE) - Optimization changes to improve delivery speed Nov 15, 2025
@paulmurdoch19

Copy link
Copy Markdown
Collaborator

was this ticket not about the table one? If not we should add that to this task since it has the same issue and I believe you can use the graphql endpoint there since we just need to know which consortiums exist

@kellcuc

kellcuc commented Nov 17, 2025

Copy link
Copy Markdown
Author

@paulmurdoch19 this was actually a two part ticket.

  1. ui
  2. the performance

@paulmurdoch19 are you talking about this table: ./analysis/tools/stats/consortiums

@paulmurdoch19

Copy link
Copy Markdown
Collaborator

I thought we created this ticket because this was to slow but I might be thinking of something else

Comment thread PcdcAnalysisTools/blueprint/routes/views/survival/__init__.py Outdated
Comment thread PcdcAnalysisTools/blueprint/routes/views/survival/__init__.py Outdated
config=capp.config
)

path=capp.config['GUPPY_API'] + "/download",

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.

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.

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
      }
    ]
  }
}

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.

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.

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 PcdcAnalysisTools/blueprint/routes/views/tableOne/__init__.py
Comment thread PcdcAnalysisTools/blueprint/routes/views/tableOne/__init__.py
Comment thread PcdcAnalysisTools/blueprint/routes/views/tableOne/__init__.py
Comment thread PcdcAnalysisTools/blueprint/routes/views/survival/__init__.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants