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

feat: don't simply toss extensions #347

Merged
merged 1 commit into from
Nov 4, 2024
Merged

feat: don't simply toss extensions #347

merged 1 commit into from
Nov 4, 2024

Conversation

mikix
Copy link
Contributor

@mikix mikix commented Sep 10, 2024

  • Print a report at the end of the ETL run of all extensions stripped by resource, URL, and count.
  • Plus a report on resources skipped due to unrecognized modifier extensions.
  • Leave stub extension in place, with just the url field, so that we can inspect stripped extensions in SQL if we want.

This commit allows a bunch of extensions in use today. Scanned some sample data from Cerner and Epic to find common extensions. We'll surely add to the list going forward, to avoid annoying folks with the unknown-extension warning.

Ignore the following extensions:

Drop support for the following extension:

Fixes #346

Example output

────────────────────────────────────────────────────────────────────────────────
Unrecognized extensions dropped from resources:
 Condition                            
 └── http://example.com/blarg (1 time)
 Encounter                                                                      
 ├── http://cerner.com/long-url/extensions/estimated-financial-responsibility-am
 │   ount (1 time)                                                              
 └── http://example.com/blarg (2 times)                                         
────────────────────────────────────────────────────────────────────────────────
🚨 Resources skipped due to unrecognized modifier extensions: 🚨
 Encounter                          
 └── http://example.com/foo (1 time)
────────────────────────────────────────────────────────────────────────────────
⭐ All tasks completed successfully! ⭐

Checklist

  • Consider if documentation (like in docs/) needs to be updated
  • Consider if tests should be added

@mikix mikix force-pushed the mikix/extensions branch 4 times, most recently from 1af1fc4 to cb36c6f Compare November 4, 2024 14:52
Copy link

github-actions bot commented Nov 4, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
3595 3534 98% 98% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
cumulus_etl/deid/scrubber.py 100% 🟢
cumulus_etl/etl/cli.py 100% 🟢
cumulus_etl/etl/tasks/nlp_task.py 100% 🟢
TOTAL 100% 🟢

updated for commit: 820acfc by action🐍

- Print a report at the end of the ETL run of all extensions stripped
  by resource, URL, and count.
- Plus a report on resources skipped due to unrecognized modifier
  extensions.
- Leave stub extension in place, with just the `url` field, so that
  we can inspect stripped extensions in SQL if we want.

This commit allows a bunch of extensions in use today.
Scanned some sample data from Cerner and Epic to find common
extensions. We'll surely add to the list going forward, to avoid
annoying folks with the unknown-extension warning.

Ignore the following extensions:
- http://hl7.org/fhir/StructureDefinition/iso21090-TEL-address
- http://hl7.org/fhir/StructureDefinition/rendered-value
- http://hl7.org/fhir/us/core/StructureDefinition/us-core-direct
- https://fhir-ehr.cerner.com/r4/StructureDefinition/clinical-
  instruction
- https://fhir-ehr.cerner.com/r4/StructureDefinition/estimated-
  financial-responsibility-amount
- http://open.epic.com/FHIR/StructureDefinition/extension/birth-
  location

Drop support for the following extension:
- http://hl7.org/fhir/us/core/StructureDefinition/us-core-sex-for-
  clinical-use (this was in US Core 6.0 ballot, but didn't make final
  cut - it seems to be replaced by just 'us-core-sex')
@mikix mikix changed the title WIP: don't simply toss extensions feat: don't simply toss extensions Nov 4, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry that there's some refactoring noise - I basically tweaked how the recursive scrubbing worked - basically just reduced the amount of full-object access each scrubber method had.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These test files are for the ms-tool tests, which now no longer strips the extensions - so these changes just add the input extensions to the expected output.

@mikix mikix marked this pull request as ready for review November 4, 2024 17:53
@mikix mikix merged commit 10720a8 into main Nov 4, 2024
3 checks passed
@mikix mikix deleted the mikix/extensions branch November 4, 2024 18:30
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.

Print extensions and modifierExtensions before stripping them
2 participants