Convert cql-execution to async #271

mgramigna · 2022-06-23T17:26:50Z

~~The behemoth has arrived. I'm putting this into draft, because I think #270 should get merged first, which greatly improves the performance of our async code.~~

Justification

NOTE: A lot of this justification might reference FHIR-specifc things. But the benefits of an asynchronous cql-execution engine theoretically apply to any data model that interfaces with cql-execution

The main motivation for converting cql-execution to async was to allow data providers to implement methods that use asynchronous operations to retrieve patient data (e.g. HTTP calls, database lookups, etc.)
Currently, all of the exec functions for ELM expressions are synchronous, which in turn means that all of the data for a given patient needs to live in memory of the engine/data provider during execution-time of the ELM.

We've been doing some experimentation with a data source that uses MongoDB to retrieve patient data at runtime from a given Mongo database, as we think this could lead to some cool implementations of data providers for cql-execution, and some cool performance optimizations for a large amount of patient data.

The way cql-exec-fhir is written right now, it requires all of the Patient Bundles to be passed in to the PatientSource class
before execution happens. This can quickly cause the JavaScript heap to run out of memory when we reach a vast amount of patients.

The burden of migrating to an asynchronous usage from a user-perspective is low: just use async/await or .then

Summary

This PR does two main things:

Convert all ELM expressions' exec methods to be async. This is necessary because we cannot make assumptions about where in the ELM expression tree asynchronous calls might happen. Basically, since a Retrieve (which could now be doing async stuff)
could be anywhere in the tree, the whole tree should be able to bubble up the Promise it returns and await it
Convert calls to findRecords/currentPatient/nextPatient to async. This keeps backwards compatibility with existing data sources, while also allowing new data sources to return Promises instead of just raw data

Because of a combo of 1. and 2., the main Executor class that is exposed by cql-execution needs to be called in an asynchronous pattern as well

Code Changes

Update all src/elm/* to use async functions
Add await to any code that uses the above functions (including in tests)
Added smart detection of Promises from currentPatient/nextPatient iteration and await when a Promise is returned by the data source for those functions
Update src/runtime/context.ts to use async findRecords calls
Update examples and documentation to reflect async pattern and added migration guide document

Testing Guidance

Run all tests and run examples in the examples directory to see that the results are the same as the synchronous version
For proof that asynchronous patient sources work, see this example repo I created that uses cql-execution and a fork of cql-exec-fhir to pull patient data from a FHIR server

Advice

Have fun

Pull requests into cql-execution require the following.
Submitter and reviewer should ✔ when done.
For items that are not-applicable, mark "N/A" and ✔.

CDS Connect and Bonnie are the main users of this repository.
It is strongly recommended to include a person from each of those projects as a reviewer.

Submitter:

This pull request describes why these changes were made
Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
Tests are included and test edge cases
Tests have been run locally and pass
Code coverage has not gone down and all code touched or added is covered.
Code passes lint and prettier (hint: use npm run test:plus to run tests, lint, and prettier)
All dependent libraries are appropriately updated or have a corresponding PR related to this change
cql4browsers.js built with npm run build:browserify if source changed.

Reviewer:

Name:

Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
The tests appropriately test the new code, including edge cases
You have tried to break the code

codecov-commenter · 2022-09-26T20:08:14Z

Codecov Report

Merging #271 (75503c6) into next (ee1ad13) will decrease coverage by 0.09%.
The diff coverage is 91.54%.

@@            Coverage Diff             @@
##             next     #271      +/-   ##
==========================================
- Coverage   86.21%   86.12%   -0.10%     
==========================================
  Files          50       50              
  Lines        4397     4426      +29     
  Branches     1233     1243      +10     
==========================================
+ Hits         3791     3812      +21     
- Misses        319      321       +2     
- Partials      287      293       +6

Impacted Files	Coverage Δ
src/elm/literal.ts	`100.00% <ø> (ø)`
src/elm/parameters.ts	`80.95% <0.00%> (ø)`
src/elm/quantity.ts	`100.00% <ø> (ø)`
src/elm/ratio.ts	`66.66% <ø> (ø)`
src/runtime/context.ts	`81.02% <ø> (ø)`
src/elm/structured.ts	`84.21% <50.00%> (ø)`
src/elm/reusable.ts	`71.25% <66.66%> (ø)`
src/runtime/executor.ts	`69.56% <66.66%> (-0.89%)`	⬇️
src/elm/interval.ts	`79.07% <77.77%> (-1.09%)`	⬇️
src/elm/clinical.ts	`82.50% <78.57%> (+0.44%)`	⬆️
... and 19 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cmoesel

This is really great work, @mgramigna! It's tedious, but important! I'd still like to do some more testing with it, but I've done a pass through the code and left a few comments. They're all very minor and should be pretty simple to resolve.

Once I've had a chance to more thoroughly test this w/ some real-world implementations, I'll give my official approval.

Regarding branches, I'd suggest that we update next to point to the HEAD of master (since next doesn't have any of its own commits yet), then rebase this on next. I think you have the proper permissions to do that, but let me know if you find otherwise.

src/elm/query.ts

src/elm/type.ts

src/runtime/executor.ts

src/util/util.ts

test/elm/aggregate/aggregate-test.ts

test/elm/external/external-test.ts

test/util/util-test.ts

V2_to_V3_MIGRATION.md

* WIP: initial async conversion in source code * Asyncified elm date-tests * Asyncified elm datetime-tests * Fixed rejection test for Today() * asyncified list-tests. fixed missing Promise.all and added some types mainly to the builder and base expression class while hunting down the issue * interval tests converted (some issues) * asyncified elm parameters-test * asyncified elm external-test (shows issues) * fixed should issues in elm external-test * asyncified elm library-test. (shows errors caused by sort issues) * Asyncified elm instance-test * Asyncified elm structured-test * Asyncified elm nullological-test * asyncified elm message-test * asyncify elm executor-test * more efficient use of Promise.all in List expression * added async/await for clinical tests * removed unnecessary async/awaits * changed should() syntax * Asyncified elm string-test * updated for async * Asyncified elm type-test * async-ify convert-test and fix convert bug * Asyncified elm query-test * added missing await for query relationship resolution in query expression processing * updated for async functionality * Update arithmetic-test.ts * Update arithmetic-test.ts * Update arithmetic-test.ts * Update test/elm/arithmetic/arithmetic-test.ts Co-authored-by: hossenlopp <hossenlopp@mitre.org> * Apply suggestions from code review Co-authored-by: hossenlopp <hossenlopp@mitre.org> * Update arithmetic-test.ts * Apply suggestions from code review Co-authored-by: Matthew Gramigna <matthewgramigna@gmail.com> * Update arithmetic-test.ts * asyncify aggregate test * asyncify literal and reusable * remove erroneous asyncs * Update comparison-test.ts * add parens * add async sorting functionality for queries * remove verified checked TODOs * regen cql4browsers * Update browser and node examples to async * update docs to async * Use new should syntax in example unit test documentation Co-authored-by: natjoe4 <35576465+natjoe4@users.noreply.github.com> * Switch asyncMergeSort to use generics and add more tests Co-authored-by: Chris Hossenlopp <hossenlopp@mitre.org> Co-authored-by: sarahmcdougall <smcdougall@mitre.org> Co-authored-by: natjoe4 <njjones@mitre.org> Co-authored-by: mriley <mriley@mitre.org> Co-authored-by: MegRiley <53576881+MegRiley@users.noreply.github.com> Co-authored-by: natjoe4 <35576465+natjoe4@users.noreply.github.com>

Co-authored-by: Chris Moesel <cmoesel@users.noreply.github.com>

In testing, I discovered that some queries do not operate correctly when each iteration of the query is being executed asynchronously. It seems related to how aliases and contexts are handled. Instead of allowing a query to run over the list asynchronously, force synchronous iteration to avoid these issues.

When two items compare as equal, the sort should preserve the original order of the items relative to each other. In other words, if item A and item B are deemed equivalent by the sort comparator, and item A appeared before item B in the original unsorted list, then item A should continue to appear before item B in the sorted list. This makes for a more predictable stable ordering.

…version

cmoesel

OK! This all looks splendid. Code is clean, regressions run fine. We're all good!

Thanks so much for this @mgramigna and team! As a close friend of mine once said:

That's one small step for man, one giant leap for mankind.

This commit contains all work done to convert the source code to async. Individual commits below show the progress of first converting over the source code and then fixing all of the unit tests as well as other issues that surfaced due to the underlying source code changes. * WIP: initial async conversion in source code * Asyncified elm date-tests * Asyncified elm datetime-tests * Fixed rejection test for Today() * asyncified list-tests. fixed missing Promise.all and added some types mainly to the builder and base expression class while hunting down the issue * interval tests converted * asyncified elm parameters-test * asyncified elm external-test * fixed should issues in elm external-test * asyncified elm library-test (shows errors caused by sort issues) * Asyncified elm instance-test * Asyncified elm structured-test * Asyncified elm nullological-test * asyncified elm message-test * asyncify elm executor-test * more efficient use of Promise.all in List expression * added async/await for clinical tests * removed unnecessary async/awaits * changed should() syntax * Asyncified elm string-test * Asyncified elm logical-test * Asyncified elm type-test * async-ify convert-test and fix convert bug * Asyncified elm query-test * added missing await for query relationship resolution in query expression processing * asyncify aggregate test * asyncify literal and reusable * remove erroneous asyncs * Update comparison-test.ts * add parens * add async sorting functionality for queries * remove verified checked TODOs * regen cql4browsers * Update browser and node examples to async * update docs to async * Use new should syntax in example unit test documentation * Switch asyncMergeSort to use generics and add more tests * Add detection of promises from patient source and await them * Add V2_to_V3_MIGRATION document and update README example * Update terminology to allow for async providers * Remove uses of isPromise and always use await * Use better function typing in multi-source query forEach * Add documentation about terminology provider async to migration guide * Force synchronous iteration where it might matter In testing, I discovered that some queries do not operate correctly when each iteration of the query is being executed asynchronously. It seems related to how aliases and contexts are handled. Instead of allowing a query to run over the list asynchronously, force synchronous iteration to avoid these issues. * Preserve original order for equivalent items in async merge sort When two items compare as equal, the sort should preserve the original order of the items relative to each other. In other words, if item A and item B are deemed equivalent by the sort comparator, and item A appeared before item B in the original unsorted list, then item A should continue to appear before item B in the sorted list. This makes for a more predictable stable ordering. * Separate out construction of expression and execution in ELM type conversion Co-authored-by: Chris Hossenlopp <hossenlopp@mitre.org> Co-authored-by: sarahmcdougall <smcdougall@mitre.org> Co-authored-by: natjoe4 <njjones@mitre.org> Co-authored-by: mriley <mriley@mitre.org> Co-authored-by: Chris Moesel <cmoesel@mitre.org>

mgramigna force-pushed the async-implementation branch from 5e268f7 to 38499aa Compare July 25, 2022 16:50

mgramigna changed the base branch from master to next July 25, 2022 16:50

mgramigna marked this pull request as ready for review July 25, 2022 16:50

mgramigna requested a review from cmoesel July 25, 2022 16:53

mgramigna assigned cmoesel Jul 25, 2022

cmoesel reviewed Sep 27, 2022

View reviewed changes

mgramigna and others added 7 commits September 27, 2022 19:41

Add detection of promises from patient source and await them

e0f3b4d

Add V2_to_V3_MIGRATION document and update README example

d3a38aa

Update terminology to allow for async providers

d27bb8e

Remove uses of isPromise and always use await

3517f01

Use better function typing in multi-source query forEach

3a15ef9

Co-authored-by: Chris Moesel <cmoesel@users.noreply.github.com>

Add documentation about terminology provider async to migration guide

00fb618

mgramigna force-pushed the async-implementation branch from 53c900d to 00fb618 Compare September 27, 2022 23:41

cmoesel and others added 3 commits October 3, 2022 15:19

Separate out construction of expression and execution in ELM type con…

ee246da

…version

mgramigna requested a review from cmoesel October 4, 2022 16:34

cmoesel changed the base branch from next to master October 4, 2022 17:30

cmoesel changed the base branch from master to next October 4, 2022 17:30

cmoesel approved these changes Oct 4, 2022

View reviewed changes

mgramigna merged commit 64cee0d into cqframework:next Oct 4, 2022

mgramigna deleted the async-implementation branch October 4, 2022 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert cql-execution to async #271

Convert cql-execution to async #271

mgramigna commented Jun 23, 2022 •

edited

Loading

codecov-commenter commented Sep 26, 2022 •

edited

Loading

cmoesel left a comment

cmoesel left a comment

Convert cql-execution to async #271

Convert cql-execution to async #271

Conversation

mgramigna commented Jun 23, 2022 • edited Loading

Justification

Summary

Code Changes

Testing Guidance

Advice

codecov-commenter commented Sep 26, 2022 • edited Loading

Codecov Report

cmoesel left a comment

Choose a reason for hiding this comment

cmoesel left a comment

Choose a reason for hiding this comment

mgramigna commented Jun 23, 2022 •

edited

Loading

codecov-commenter commented Sep 26, 2022 •

edited

Loading