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

Add RR extraction to PHDI SDK #800

Merged
merged 18 commits into from
Sep 7, 2023
Merged

Add RR extraction to PHDI SDK #800

merged 18 commits into from
Sep 7, 2023

Conversation

emmastephenson
Copy link
Collaborator

PULL REQUEST

Summary

Moves the RR extraction logic from the ReadSourceData function app in phdi-azure to the SDK.

Related Issue

Part 1 of #737

Additional Information

Order of PRs here will be roughly:

  1. This function is moved to SDK
  2. FHIR Converter and Validation containers are updated to accept both eICR and RR data, then call this SDK function
  3. Function app in phdi-azure calls this SDK function
  4. (Eventually) LAC pipeline is updated to send both eICR and RR data to validation BB/FHIR Converter

@emmastephenson emmastephenson changed the base branch from main to emma/737-rr-extraction August 31, 2023 22:09
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #800 (717f484) into main (fd593ea) will increase coverage by 0.06%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #800      +/-   ##
==========================================
+ Coverage   96.35%   96.42%   +0.06%     
==========================================
  Files          45       45              
  Lines        2576     2626      +50     
==========================================
+ Hits         2482     2532      +50     
  Misses         94       94              
Flag Coverage Δ
unit-tests 96.42% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
phdi/fhir/conversion/__init__.py 100.00% <100.00%> (ø)
phdi/fhir/conversion/convert.py 99.09% <100.00%> (+0.73%) ⬆️

Base automatically changed from emma/737-rr-extraction to main August 31, 2023 22:45
@@ -13,7 +13,7 @@ python = "^3.10"
smartystreets-python-sdk = "^4.10.6"
pydantic = "^1.10.9"
fastapi = "^0.96.0"
httpx = "^0.23.3"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to update HTTPX because otherwise phdi couldn't be added as a dependency of the FHIR converter container.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have been trying not to add phdi as a dependency in the FHIR converter to avoid making a larger image even larger. If we need to do this for now I think that is fine, but I'd like to have a conversation about breaking the SDK into smaller packages, each published to pypi separately so that each BB can be more selective in terms of what parts of the SDK to include as dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good catch actually! I think breaking up the containers/sdk into smaller packages is a good way to go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happy to chat more about this offline! But for now I'm in favor of importing phdi in the FHIR converter as-is. If we decide to break up the SDK, I think that should be a separate effort.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, my thought is that as the project continues to evolve the single SDK model doesn't really fit our current use case.

@BradySkylight
Copy link
Contributor

Your point 4. (Eventually) LAC pipeline is updated to send both eICR and RR data to validation BB/FHIR Converter may be incorrect. If the read source data function in the LAC pipeline already grabs both eICR and RR then they can just pass those into the validator, as they do now, but then after that step only a single eICR message with the RR should be passed back, as we don't want to necessarily repeat the extraction process in both BBs, if that makes sense. Then the FHIR Converter would still just receive a single eICR message for LAC pipeline. However, we want this extraction process in both BBs to keep them completely modular. Let me know if that makes sense.

Copy link
Collaborator

@DanPaseltiner DanPaseltiner left a comment

Choose a reason for hiding this comment

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

🚀

@emmastephenson emmastephenson merged commit d4f41e1 into main Sep 7, 2023
15 checks passed
@emmastephenson emmastephenson deleted the emma/737-really branch September 7, 2023 15:23
@nickclyde nickclyde mentioned this pull request Sep 26, 2023
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