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 MERIndicators CQL #39

Open
citizenrich opened this issue Jul 5, 2024 · 7 comments
Open

Run MERIndicators CQL #39

citizenrich opened this issue Jul 5, 2024 · 7 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@citizenrich
Copy link

Hi,

Thanks for the great app. I'm just trying it out and see an error:

CQL CLI failed with an error: failed to parse CQL: failed to parse library: , failed to import library "DAKConcepts", dependency graph could not resolve with error: DAKConcepts  does not exist in the graph.

The repository I'm using is https://github.com/PATH-Global-Health/MERIndicators and the invocation I'm using is:

cql -cql_dir="input/cql" -fhir_bundle_dir="input/tests" -fhir_terminology_dir="input/vocabulary" -json_output_dir="./"

I'm not clear about the error, if there's any guidance that can be provided to help me troubleshoot it. Thanks.

@suyashkumar
Copy link
Contributor

@citizenrich thank you for raising this! At first glance, this appears to be happening because our library importing logic (topological sort during library import) does not properly support including libraries without a version.

For example:
https://github.com/PATH-Global-Health/MERindicators/blob/6e6403176ff980776bcfe8b697fedbd4e1939839/input/cql/DAKDataElements.cql#L7C1-L7C8

This include DAKConcepts called Cx is causing our library issues. You can bypass this issue temporarily by passing an explicit version in each include. Either way, this is an issue we should fix on our end (likely next week), and I'll update this issue title to reflect this issue.

Thanks again for raising!

@suyashkumar suyashkumar changed the title CLI failed with an error: failed to parse CQL: failed to parse library Include statements without version should be supported Jul 5, 2024
@suyashkumar
Copy link
Contributor

Note, that after updating all the includes with a version, there's still an error running your CQL with our engine:

2024/07/05 23:31:47 CQL CLI failed with an error: failed to parse CQL: 129-3 could not resolve Equivalent(Named<FHIR.CodeableConcept>, System.Code): no matching overloads
130-5 could not resolve Equivalent(Named<FHIR.CodeableConcept>, System.Code): no matching overloads
131-5 could not resolve Equivalent(Named<FHIR.CodeableConcept>, System.Code): no matching overloads

As noted in the README, this toolkit is pretty early and fully experimental with missing system operators and overloads.

For this one, I this this may be because there's no implicit converstion between FHIR.CodeableConcept and System.Code in CQL. Without looking too deeply, I think there's a conversion between FHIR.Coding and System.Code, but not one between CodeableConcept and System.Code. So this might need to be something you update in the code where this comparison is happening? If this runs in other engines, then it could very well be something we're not doing right. Our equivalent overload is a (T, T) overload meaning that we expect to be able to implicitly convert both operands to the same type.

@suyashkumar suyashkumar added bug Something isn't working enhancement New feature or request labels Jul 5, 2024
@citizenrich
Copy link
Author

Thanks for the great troubleshooting! It makes sense about the includes and authors can make that fix easily. The code does run on the CQF tooling engine and the HAPI FHIR JPA with clinical reasoning module enabled. But, I don't know enough about the codeable concept issue to offer anything useful, so I'll ping @brynrhodes

@suyashkumar
Copy link
Contributor

suyashkumar commented Jul 6, 2024

No problem! Sounds good, and thank you for trying our tools out! :)

I took a closer look at the Equivalent issue, and looks like that's just something we need to do on our end. Equivalent has both (T, T) overloads and some additional explicit clinical overloads (where (System.Concept, System.Code) is a valid overload. So we'll also prioritize that next week! #40 to track that.

@evan-gordon
Copy link
Collaborator

@citizenrich thanks again for your report!

I merged in @suyashkumar's change to add libraries to the error output, hopefully that will make debugging smoother in the future.

I started work today on #40 I'd like to get that merged in early next week depending on team availability.

copybara-service bot pushed a commit that referenced this issue Jul 8, 2024
This also corrects the Equivalent(Code, Code) operators to use equivalence semantics when comparing the underlying system and code string values.

Support for Equivalent(Code, Concept) coming in a fast-follow.

This addresses one of the issues in #39, and part of #40.

PiperOrigin-RevId: 650283783
copybara-service bot pushed a commit that referenced this issue Jul 8, 2024
This also corrects the Equivalent(Code, Code) operators to use equivalence semantics when comparing the underlying system and code string values.

Support for Equivalent(Code, Concept) coming in a fast-follow.

This addresses one of the issues in #39, and part of #40.

PiperOrigin-RevId: 650323027
@suyashkumar
Copy link
Contributor

I sent 33faab5 for the (Concept, Code) overload earlier (and fixed the (Code, Code)) one. Across Evan and I, we'll try to wrap up the other overloads.

That said, I think I'll just change the title of this issue to "Run MERIndicators" since there are a few other minor associated things we can fix to support all the CQL in there. It's as good of a target as any to get ship ship, particularly if @citizenrich is interested in running it with our engine! :)

@suyashkumar suyashkumar changed the title Include statements without version should be supported Run MERIndicators CQL Jul 9, 2024
copybara-service bot pushed a commit that referenced this issue Jul 9, 2024
Required for the CQL in #39.

PiperOrigin-RevId: 650466292
copybara-service bot pushed a commit that referenced this issue Jul 9, 2024
Required for the CQL in #39.

PiperOrigin-RevId: 650702309
@suyashkumar
Copy link
Contributor

suyashkumar commented Jul 10, 2024

Here are some items we'll need to support for this CQL (can break the larger ones out into separate tasks if we want):

  • Combine Operator
  • Min Support (DateTime)
  • Max Support (DateTime)
  • Overlaps interval
  • Indexer
  • Support for referencing ExpressionDefinitions that have not yet been created (this CQL will reference "Exp1" before "Exp1" has been declared--though that can be fixed on the authorship side too).
  • Support FHIR Path functions that have matching CQL functions (specifically now())
  • Support query filtering on CodeRef

Feel free to edit this comment and add some more if needed.

copybara-service bot pushed a commit that referenced this issue Jul 10, 2024
This is needed for #39.
This fixes #49.

PiperOrigin-RevId: 651173411
copybara-service bot pushed a commit that referenced this issue Jul 11, 2024
This is needed for #39.
This fixes #49.

PiperOrigin-RevId: 651173411
copybara-service bot pushed a commit that referenced this issue Jul 11, 2024
This is needed for #39.
This fixes #49.

PiperOrigin-RevId: 651204863
copybara-service bot pushed a commit that referenced this issue Jul 11, 2024
This addresses part of #39.

PiperOrigin-RevId: 651178839
copybara-service bot pushed a commit that referenced this issue Jul 11, 2024
This addresses part of #39.

PiperOrigin-RevId: 651237467
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants