OHIF / Viewers

OHIF zero-footprint DICOM viewer and oncology specific Lesion Tracker, plus shared extension packages
https://docs.ohif.org/
MIT License
3.12k stars 3.29k forks source link

[Feature Request] Support of two DICOM servers #3507

Closed fedorov closed 5 months ago

fedorov commented 1 year ago

What feature or change would you like to see made?

Add support to configure the viewer with 2 DICOM servers as sources, so that when a study is selected, series from that study are retrieved from each of the servers.

Why should we prioritize this feature?

In the situations where large number of images are analyzed, it is beneficial to be able to review the analysis results/annotations without having to contribute those into the same DICOM server where the images are. In some cases, this is not possible because the server containing the images is "read only" (e.g., for the content available from NCI Imaging Data Commons). In other cases, it is just not desirable to combine those annotations/analysis results with the images, since their quality may first need to be established, as an example. Replicating images that were analyzed in a separate server can be a very costly and time-consuming procedure (e.g., there are image collections with the size measured in terabytes in IDC).

In v2, we implemented experimental support for this feature in a fork (see this branch https://github.com/ImagingDataCommons/Viewers/tree/IDC2servers and https://github.com/OHIF/Viewers/issues/1512), and it proved to be extremely useful. We use this feature routinely for the development work. Without this feature, switching from v2 to v3 for us will be very disruptive.

igoroctaviano commented 1 year ago

@sedghi here is another item that was not part of our last meeting but should be. Any thoughts? is this something that might be on the roadmap or could be added?

sedghi commented 1 year ago

Hmmmm, interesting. I haven't looked into the code yet, but is it like when a study is opened it sends a qido to each server to see if it contains the study, and then merge the results?

We have multiple data source configuration in the immediate roadmap, which let you select the datasource in the worklist. There is another item in the roadmap to combine the results from multiple datasources in the worklist (nifti + dicom) and show each. But I guess what you are talking here is a little bit different, right?

sedghi commented 1 year ago

CC @jbocce Since he is assigned to do the multi datasource item in near future

igoroctaviano commented 1 year ago

Hmmmm, interesting. I haven't looked into the code yet, but is it like when a study is opened it sends a qido to each server to see if it contains the study, and then merge the results?

We have multiple data source configuration in the immediate roadmap, which let you select the datasource in the worklist. There is another item in the roadmap to combine the results from multiple datasources in the worklist (nifti + dicom) and show each. But I guess what you are talking here is a little bit different, right?

Here's the simplest use case I can think of:

This way we can retrieve other series from other servers without having to copy the derived series to the original/main image server or configure a new server with them combined.

Right now we could keep this even simpler and just support this if they are dicomweb data sources (no need to merge nifti with dicom or something else at the moment)

@fedorov please correct me if I'm wrong

sedghi commented 1 year ago

I understand the use case, but to me the feature (multi dicom server) should not care about if there some series are derived or not. It should just be a secondary call to retrieve anything from the secondary server as well.

igoroctaviano commented 1 year ago

I understand the use case, but to me the feature (multi dicom server) should not care about if there some series are derived or not. It should just be a secondary call to retrieve anything from the secondary server as well.

This is just a concrete use case, I'm not saying it has to worry about it being derived or not, it could be anything.

fedorov commented 1 year ago

is it like when a study is opened it sends a qido to each server to see if it contains the study, and then merge the results?

Yes!

We have multiple data source configuration in the immediate roadmap, which let you select the datasource in the worklist. There is another item in the roadmap to combine the results from multiple datasources in the worklist (nifti + dicom) and show each. But I guess what you are talking here is a little bit different, right?

Except that we couldn't care less about nifti - yes!

I understand the use case, but to me the feature (multi dicom server) should not care about if there some series are derived or not. It should just be a secondary call to retrieve anything from the secondary server as well.

Correct!

@igoroctaviano your description is correct.

Again - no niftis anywhere close for our use case or for IDC-motivated functionality, please!

igoroctaviano commented 1 year ago

@rodrigobasilio2022

rodrigobasilio2022 commented 1 year ago

I added today in the PR the ability to define a second server with the URL parameter secondGoogleServer.

The following pictures demonstrate this functionality.

Main datastore, with two studies. Note that the expanded study has three series.

image

Opening the study using an URL with the secondGoogleServer parameter, it shows four series, the dicom seg series belonging only to the second server

image

Next picture shows that the parameter SeriesInstanceUID works to filter a particular series. Remember that this url parameter works as follows: it tries to filter the series, but if the result is empty, it returns all series found of a particular study. That's why the segmentation series also appears

image

rodrigobasilio2022 commented 1 year ago

Note there is a difference from v2: In v2 the parameter was preceded by !

http://localhost:3000/viewer/1.3.6.1.4.1.14519.5.2.1.6655.2359.247426265538388028079845922798!secondGoogleServer=/projects/idc-tcia/locations/us-central1/datasets/tcia-idc-datareviewcoordination/dicomStores/planar_annotations

In v3 it is a normal html parameter, as you can see in the example: http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.99.1071.26968527900428638961173806140069&secondGoogleServer=projects/idc-external-002/locations/us-west1/datasets/twoserversTest/dicomStores/test&SeriesInstanceUID=1.3.6.1.4.1.14519.5.2.1.99.1071.24930372521077247896408083282537

igoroctaviano commented 1 year ago

I added today in the PR the ability to define a second server with the URL parameter secondGoogleServer.

The following pictures demonstrate this functionality.

Main datastore, with two studies. Note that the expanded study has three series.

image

Opening the study using an URL with the secondGoogleServer parameter, it shows four series, the dicom seg series belonging only to the second server

image

Next picture shows that the parameter SeriesInstanceUID works to filter a particular series. Remember that this url parameter works as follows: it tries to filter the series, but if the result is empty, it returns all series found of a particular study. That's why the segmentation series also appears

image

I reviewed the code, let's schedule a call first. I think we need to abstract things a bit (take gcp-specific things out) otherwise it won't make to master.

rodrigobasilio2022 commented 1 year ago

@igoroctaviano and I discussed, I implemented and the present solution is general enough and allows multiple server definitions. @igoroctaviano is giving the first review

fedorov commented 1 year ago

How does the series filter work - is it applied to both servers, or to only one of them?

rodrigobasilio2022 commented 1 year ago

Now it's applied to both servers.

igoroctaviano commented 11 months ago

@rodrigobasilio2022 whats the status on this one?

rodrigobasilio2022 commented 11 months ago

Had two approvals. I think just depeding on Ali to approve and merge

igoroctaviano commented 11 months ago

Had two approvals. I think just depeding on Ali to approve and merge

Please follow up with him to get it merged. Thanks!

sedghi commented 6 months ago

@igoroctaviano I think this is finished right?

igoroctaviano commented 6 months ago

@igoroctaviano I think this is finished right?

Yes, its finished.

sedghi commented 5 months ago

We just release the OHIF 3.8, you can find more details here https://ohif.org/release-notes/3p8/ If you still encounter this issue in 3.8, please re-open this.