Skip to content
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.

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

Run all creative mode templates simultaneously / fix and adjust record-counting for queries and template execution #824

Closed
tokebe opened this issue Jun 24, 2024 · 10 comments
Assignees
Labels
debt Issue represents some technical debt On Test Related changes are deployed to Test server

Comments

@tokebe
Copy link
Member

tokebe commented Jun 24, 2024

As a likely improved alternative to #808, we should instead try running all templates simultaneously.

This slightly complicates response merging, but race conditions should be solvable with a per-query execution lock.

More importantly, we still don't want to go over time, so we want to time out at about 4:55 to ensure there's time to finish up and make callbacks/store the response. This might be both on individual templates and on the entire handler.

After implementation, we'll want some performance testing to ensure this doesn't create unreasonable strain on memory/CPU.

@rjawesome
Copy link
Contributor

See PRs

@tokebe tokebe added the debt Issue represents some technical debt label Jun 27, 2024
@rjawesome
Copy link
Contributor

Note: It's likely that the current code for this PR will conflict with #825 because the code here aborts all subqueries when the time cap is reached. However, it is unclear how to handle this when the subqueries are in a queue seperate from the thread (and all subqueries from different queries are merged into one queue).

@tokebe
Copy link
Member Author

tokebe commented Jul 2, 2024

Regarding integrating with #825, in that case we don't want to abort subqueries because we'd prefer them to keep running and cache results whether those results are being used immediately or not. So, if a template adds subqueries to the queue, and then the template execution is aborted, we'd want those subqueries to keep running -- it increases the chances the template will complete before timeout in subsequent queries to BTE.

@tokebe
Copy link
Member Author

tokebe commented Jul 2, 2024

This being the case, after you've verified #825 performs well under high traffic, you might want to rebase branches addressing #825 to branches for this issue, so you can seamlessly merge the behavior of the two issues. Normally we wouldn't do this, but because both are very involved changes that will be going up simultaneously (and theoretically support one another, performance-wise), it makes sense to do so.

@tokebe
Copy link
Member Author

tokebe commented Sep 3, 2024

@rjawesome please update PRs to merge in Pathfinder behavior from main (currently has a merge conflict, may require extra changes)

@rjawesome
Copy link
Contributor

@tokebe done.

@colleenXu
Copy link
Collaborator

colleenXu commented Oct 22, 2024

Fixes/logging-improvements were deployed to CI using commits to main branches:


Related: changed per-query/per-template record max. Functionally increased max from 50k to 75k.
biothings/call-apis.js@501df36

@colleenXu colleenXu changed the title Run all creative mode templates simultaneously Run all creative mode templates simultaneously / fix and adjust record-counting for queries and template execution Oct 22, 2024
@colleenXu
Copy link
Collaborator

After discussing urgent timeout / "large response" problems with @tokebe, we decided on the following quick-changes to this feature (not extensively tested):

  • Parallel template execution only: stop/terminate earlier (4:30 rather than 4:45)
  • Drop record limit closer to what it was before/same as before (50k)

Note: large response maybe related to duplicated edge-attribute sets #891, but not sure.

We can walk some of these changes back later after figuring out they're okay.

@colleenXu
Copy link
Collaborator

colleenXu commented Oct 22, 2024

Commits:

@colleenXu colleenXu added On CI -> Test and removed On CI Related changes are deployed to CI server labels Oct 23, 2024
@tokebe tokebe added On Test Related changes are deployed to Test server and removed On CI -> Test labels Oct 28, 2024
@tokebe tokebe closed this as completed Dec 3, 2024
@tokebe
Copy link
Member Author

tokebe commented Dec 3, 2024

Related changes deployed to Prod as of 11/13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Issue represents some technical debt On Test Related changes are deployed to Test server
Projects
None yet
Development

No branches or pull requests

3 participants