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.

Sign up for GitHub

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

Jump to bottom

ehr-api becomes export-api; losing and gaining some features #370

Merged
merged 59 commits into from
Apr 30, 2024
Merged

ehr-api becomes export-api; losing and gaining some features #370

merged 59 commits into from
Apr 30, 2024

Conversation

jeremyestein
Copy link
Contributor

@jeremyestein jeremyestein commented Apr 11, 2024 •
edited by stefpiatek
Loading

Fixes #321

DICOMWeb uploads are known not to work; this is being fixed in #381

Git hasn't detected some files as renames, so you may wish to do:
git difftool main:pixl_ehr/src/pixl_ehr/main.py jeremy/export-api:pixl_export/src/pixl_export/main.py
git difftool main:pixl_ehr/tests/test_ehr_processing.py jeremy/export-api:pixl_export/tests/test_export_processing.py
etc.

TODO:

  • Fix up various readmes
  • Check test coverage hasn't changed for the worse (maybe deleted tests could be reapplied somewhere else?)
  • Add test for anon->export API call
  • Do the tests I deleted related to the ehr_ tables need to be applied to the image table instead?
  • Find the "top level document for step naming" (see issue)
  • Work out what needs to be done with the radiology linker table
  • Convert remaining bits to loguru?
  • Rename _EHR_ env variables

@jeremyestein
Rename extract-radiology-reports CLI command to export-patient-data t…
ed6edb9 
…o better reflect what it

now does, and to match the API endpoint name.
@jeremyestein
Another rename
f2f867a
@jeremyestein
Move most of the code in the orthanc-anon plugin into
146f513 
ehr-api/export-api, plus a bit of config to get it working there.
Currently just moved into main module; will need organising
into modules and other fixes in due course.
@jeremyestein
Check HTTP return code in dicom-web upload (not used yet)
42a8b88
@jeremyestein
Use fastapi properly and fix some typing errors
9fc091f
@jeremyestein
Only FTP code should have been moved
5342333
@jeremyestein
Keep main tidy
410b718
@jeremyestein
Remove code relating to querying Emap for radiology reports
c3f8cbd
@jeremyestein
Merge branch 'main' into jeremy/export-api
36ff768
@jeremyestein
Delete large amount of code relating to querying Emap Star and Cogstack
1f1cde0
@jeremyestein
Delete fake emap star
224a04e
@jeremyestein
Rename ehr api to export api (except config env vars)
f0b466f
@jeremyestein
Remove ehr rabbitmq queue
f6ddb87
@jeremyestein
Merge branch 'main' into jeremy/export-api
762bd5e
@jeremyestein
Remove pixl az-copy-ehr command and related code
9e95f9b
@jeremyestein
docs fix
041842b
@jeremyestein
Merge branch 'main' into jeremy/export-api
eee2541
@jeremyestein
Merge branch 'main' into jeremy/export-api
3451546
@jeremyestein
Merge branch 'main' into jeremy/export-api
347f25c
@jeremyestein
First go at making the radiology linker table. The DB lookup is failing
0f04488 
due to slugify confusion.
@jeremyestein
Use CliRunner so can debug in IDE more easily
02ffaa3
@jeremyestein
Clarify raw and slugified names
84c0677
@jeremyestein
Tidy logging
3b4de7c
@jeremyestein
Fix mypy error
138c45d
@jeremyestein
Rename
b985e40
@jeremyestein
Refactor the test a bit
7962df2
@jeremyestein
Add test to detect missing radiology linker table, and a couple of
666d604 
typing and other errors
@jeremyestein
Use CliRunner for better debugging
961cc9a
@jeremyestein
CliRunner invoke does not throw on a failed command - this needs to be
d08cb5e 
checked manually and was obscuring errors, making debugging harder.
@jeremyestein
Use sqlalchemy objects better so the code actually works
426f350
@jeremyestein jeremyestein requested a review from a team April 26, 2024 15:40
milanmlft
milanmlft reviewed Apr 26, 2024
View reviewed changes
Copy link
Member

@milanmlft milanmlft left a comment

Choose a reason for hiding this comment

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

Looking great! 🚀
I think we just need to tweak the Uploader.upload_dicom_image() method a bit and make sure the Dicomweb uploader is still working.

cli/src/pixl_cli/_config.py Outdated Show resolved Hide resolved
pixl_export/src/pixl_export/main.py Show resolved Hide resolved
pixl_export/src/pixl_export/main.py Show resolved Hide resolved
@milanmlft
Copy link
Member

OK so I pushed some changes to update the DICOMweb uploader and its tests, which includes a rework done by @stefpiatek in #378. This will break core and system tests, but that's to be expected because I don't think the DicomWebUploader is working... Sorry about this, #379 shouldn't have been merged in after all 😞

peshence
peshence approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@peshence peshence left a comment

Choose a reason for hiding this comment

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

Looks good, mostly pointing out some docs that potentially need updating, feel free to resolve all of these as you see fit.

Nice work! 🚀

cli/src/pixl_cli/main.py Outdated Show resolved Hide resolved
orthanc/orthanc-anon/plugin/pixl.py Show resolved Hide resolved
orthanc/orthanc-anon/plugin/pixl.py Outdated Show resolved Hide resolved
orthanc/orthanc-anon/plugin/pixl.py Show resolved Hide resolved
pixl_core/README.md Outdated Show resolved Hide resolved
pixl_imaging/README.md Outdated Show resolved Hide resolved
test/docker-compose.yml Show resolved Hide resolved
stefpiatek
stefpiatek approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@stefpiatek stefpiatek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Really nice to see us stripping away so much. Thanks for getting this in Jeremy. Some suggestions, think integration of dicomweb will take a bit and then worth switching over to loguru for export-api

docs/file_types/parquet_files.md Outdated Show resolved Hide resolved
pixl_export/src/pixl_export/_orthanc.py Outdated Show resolved Hide resolved
cli/src/pixl_cli/main.py Outdated Show resolved Hide resolved
cli/tests/test_populate.py Show resolved Hide resolved
pixl_core/src/core/uploader/_dicomweb.py Outdated Show resolved Hide resolved
pixl_export/src/pixl_export/main.py Outdated Show resolved Hide resolved
pixl_export/src/pixl_export/main.py Show resolved Hide resolved
pixl_export/tests/test_export_processing.py Outdated Show resolved Hide resolved
test/docker-compose.yml Show resolved Hide resolved
cli/src/pixl_cli/main.py Outdated Show resolved Hide resolved
@jeremyestein jeremyestein mentioned this pull request Apr 30, 2024
Open
@jeremyestein jeremyestein mentioned this pull request Apr 30, 2024
Open
@milanmlft milanmlft mentioned this pull request Apr 30, 2024
Merged
3 tasks
@jeremyestein jeremyestein enabled auto-merge (squash) April 30, 2024 14:30
@jeremyestein jeremyestein merged commit 107c267 into main Apr 30, 2024
8 checks passed
@jeremyestein jeremyestein deleted the jeremy/export-api branch April 30, 2024 14:51
@jeremyestein jeremyestein mentioned this pull request May 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milanmlft milanmlft milanmlft left review comments

@peshence peshence peshence approved these changes

@stefpiatek stefpiatek stefpiatek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert ehr api to export api
4 participants
@jeremyestein @milanmlft @peshence @stefpiatek