-
Notifications
You must be signed in to change notification settings - Fork 1
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
CLI command to export radiology; fix system tests #227
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
path explicitly because it's different depending on where it's running from, so there is no default that makes sense.
non-slugified version into the API.
any change in the source tree forced a rebuild.
test. Radiology parquet is still empty though.
* Slim down ehr tests Reduce use of docker containers and run tests in local pytest * Make sure output file is not owned by root * Awkward workaround for running containers as current user, so that any files created on mounts have the right ownership, and because running as root is bad... * Abandon attempt to run container as non-root; this doesn't work and needs a wider fix and further consideration anyway. Just try chowning the file. * chowning doesn't work either (breaks various tests). Just delete it from within the container that created it and we'll have to come back to this. * Disable check for now as we know it fails * Can't use container after it's been stopped * wrong dir * Correct container name * Don't assume "exports" dir is part of the path, just put the dir structure exactly where we're told. * Wait for healthy containers in better way * Rearrange export directory structure so it makes more sense, and there's only one symlink, which is only created by the CLI, not EHR. * Any FileNotFoundError will do * CLI test was asserting the wrong dir was a symlink. Defensively sort the results of `glob`; the order is not guaranteed. * Minor path error * Can't check for symlink in EHR-only test - radiology export no longer creates it * Update pixl_ehr/tests/test_processing.py * Don't blanket ignore security warning * doc error --------- Co-authored-by: Jeremy Stein <j.stein@ucl.ac.uk>
This reverts commit c5e9b5c.
stefpiatek
approved these changes
Jan 26, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, most of my suggestions are fixing up existing code. Happy for you to merge once they're addressed
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TODO: