-
Notifications
You must be signed in to change notification settings - Fork 175
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
[API] Add dicom upload #9154
base: main
Are you sure you want to change the base?
[API] Add dicom upload #9154
Conversation
I must say I am not sure what I am supposed to do regarding the failed tests on files that I did not modify in the PR. Any direction @driusan (or someone else) 🤔. |
Line php/libraries/FilesDownloadHandler.php Line php/libraries/SiteIDGenerator.php |
Hum, yeah, I already saw that. I was just suprised to see that the errors are marked as originating from I'll tackle this tomorrow probably. |
I've looked at the static errors in more details, it appears to be mainly due to an upgrade of phpstan, which changes the typing of string conversions with However, I find the code that triggers the errors to be quite cryptic, with some lines that seem obviously wrong (at least according to typing). If someone who knows this code better than me could look into it, it would be best. In the meantime, I'm adding this to the next meeting. |
Probably because composer update was run, which triggered an update of the static checker. |
Ok it seems the linting errors are |
fd349f1
to
222b5d4
Compare
@ridz1208 Ok I finally finished updating and re-testing this PR. Generally it seems to work. However, if the pipeline fails during the latter stages, the DICOM archive previously inserted does not have its session initialized. This should be fixed with the new DICOM archive PR, but I don't expect it to be merged before at least a few weeks. |
4b8a6dc
to
9765668
Compare
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.
@maximemulder I did a first pass of review. I did not have a chance to actually test the changes as my MCIN sandbox is not letting me logging in. Have to figure out why and it could take some time so figured I would do an initial review.
GET /candidates/$CandID/$VisitLabel/dicoms/$Tarname/processes | ||
``` | ||
|
||
The response contains all `mri_upload` attempts with the specified `$tarname`. And for |
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.
technically, there should only be one UploadID
per TarchiveID
. So when you overwrite, you should grep the UploadID
associated to what is being overwritten instead of inserting a new entry in mri_upload
. Especially since the incoming directory content will be overwritten anyway.
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.
I agree that there is only one MRI upload per DICOM archive, but I've seen a few cases in testing (even without this PR) where there are a few failed MRI uploads and one successful for a single file. This is how the batch uploader does it (don't know about other pipelines).
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.
yeah. I think in principle, there should only be one upload per tarchive but unfortunately the batch upload script is not compliant with this set up. However, for the API, I think we should make sure this is the behaviour at minimum, to avoid duplicated entries.
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.
Is this thread resolved?
Please remove all the unrelated changes (whitespace, variable name changes, etc). If necessary you can send those in another PR. It is not feasible to review this PR with so many unrelated changed mixed together. |
65869da
to
4c63612
Compare
I replied to all the comments and applied the suggested changes. I reverted the breaking changes to the API including floats / strings and naming. Back when I wrote them, I thought it was part of the original PR and not something that was already in LORIS. I made the terminology more consistent by using I also disabled my trailing spaces auto-removal and removed the associated changes. |
"SeriesDescription" : "MPRAGE_ipat2", | ||
"SeriesNumber" : "2", | ||
"SeriesNumber" : 2, |
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.
Regarding this change, the current documentation is inaccurate, I can extract this to a separate PR if wanted.
Example:
https://demo.loris.ca/api/v0.0.4-dev/candidates/587630/V1/dicoms
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.
I need to look into this to see if it's a bug in the documentation or the code. It seems weird that it's only the SeriesNumber that does it and it might be related to PHP (not LORIS) changes in the PDO over time.
It might be faster to split it out.
6f882bc
to
8112fc8
Compare
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.
@maximemulder I re-reviewed quickly your PR. A few additional comments:
- In the description of the PR, could you add a "Notes for existing project" with the information of the SQL patches to run?
84b6398
to
8230e89
Compare
6f3651d
to
0488bb0
Compare
@cmadjar One of your comments was that you would like to the DICOM API upload to re-use an existing MRI upload if there is one. Is the following commit good for you ? I notably do not update the upload timestamp as the DICOM archive has its own timestamp and the MRI upload may be referenced by other, older, data. But I can change that if you want to. I also only re-use an existing MRI upload if its parameters are similar (upload location, uploader id...). |
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.
LGTM. Thanks for the last change!
@driusan I believe I answered all the issues that were raised by Cécile (who approved the PR) and you. I also updated the 0.0.4-dev Swagger schema to match the markdown documentation. You can review the last 7 commits commit by commit to see the changes. |
Brief summary of changes
This is a port of #6161, which contains a description of the changes. Since the old PR has diverged quite a bit from the main branch, and that its author no longer works here, I found it easier to copy the changes on the current main branch and open a new PR rather than to rebase the PR. While porting, I only rewrote the old PR code whenever static analysis or practical stests showed that it was no longer in sync with the main branch (or when my IDE removed trailing spaces).
Testing instructions (if applicable)
Notes for existing projects
This PR requires to run the following SQL patch
SQL/New_patches/2024-03_19-MRIUploadServerProcessRel.sql
.