[BEAM-11073] Dicom IO Connector for Java #13137

Aliraza-N · 2020-10-19T18:26:05Z

We are looking to create a new Java Pipeline Connector to help facilitate Reading data from the Imaging API in GCP. Initially, this connector will be used to read study level metadata of Dicom instances.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	Dataflow	Samza	Twister2
Go	---	---	---
Java
Python		---	---
XLang	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---		---	---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

This reverts commit 0cf68a2.

Aliraza-N · 2020-10-19T18:30:27Z

R: @poojavenkatram

poojavenkatram

Thanks Aliraza.

Best
Pooja

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java

...ud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HttpHealthcareApiClient.java

...o/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIOTest.java

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java

poojavenkatram

Thanks Aliraza.

Best
Pooja

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java

...o/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIOTest.java

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java

poojavenkatram

Thanks Aliraza - looking great just a few more things.

Best
Pooja

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java

poojavenkatram

Thanks Aliraza looks great just a few more docs changes.

Best
Pooja

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java

...ud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HttpHealthcareApiClient.java

Aliraza-N · 2020-10-26T18:55:42Z

R: @lukecwik
Hi, can you take a look at this PR? Thanks

pabloem · 2020-11-03T20:40:09Z

Run Java PreCommit

pabloem

Can you add an integration test? You can refer to the FhirIO PR as an example: https://github.com/apache/beam/pull/11339/files

Specifically, FhirIOReadIT.java is an example of an integration test

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java

pabloem · 2020-11-05T21:57:42Z

can you add an integration test? Feel free to ping me to discuss why/why not to add one.

Aliraza-N · 2020-11-06T14:04:35Z

I can't figure out how to reply directly to your comment above, but yes, I did start working on an integration test. Just not part of the prev commit.

Aliraza-N · 2020-11-06T19:48:01Z

Run Java PreCommit

Aliraza-N · 2020-11-09T14:20:51Z

retest this please

...google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIOReadIT.java

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java

pabloem · 2020-11-12T18:44:07Z

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java

+    @SuppressWarnings({"nullness", "rawtypes"})
+    static class FetchStudyMetadataFn extends DoFn<String, String> {
+
+      private HealthcareApiClient dicomStore;


I suspect that if you do this, you can remove the nullness condition in SuppressWarnings?

Suggested change

private HealthcareApiClient dicomStore;

private HealthcareApiClient dicomStore = new HttpHealthcareApiClient();

Maybe you also need to add null checks in process... up to you.

Hmm I'm still failing compileJava.

pabloem

Just a couple comments. Overall it's looking great.

Aliraza-N · 2020-11-13T16:23:55Z

@pabloem Hi, can you take another look please? I'm still a little uncertain about removing the nullness warning suppression. I'm thinking adding some checks should be able to catch any error.

...google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIOReadIT.java

pabloem · 2020-11-13T17:30:56Z

hm okay, feel free to re-add the suppression. Sorry about the trouble! And just the one comment about PAssert. LGTM after that.

Aliraza-N · 2020-11-13T20:06:33Z

All done! @pabloem

aaltay · 2020-11-25T20:55:13Z

What is the next step for this PR?

Aliraza-N · 2020-11-26T14:13:53Z

Hey, sorry this PR has been sitting here for a while. I have a couple of more commits (refactoring, clean up) for this PR. Should be on the tail-end of its life. I'll push these commits and notify the reviews in a few hours.

Aliraza-N · 2020-11-26T19:43:20Z

@pabloem Hey I've made some changes after some feedback on my pipeline project. Can you take a look when you have a chance? thanks

pabloem · 2020-11-30T17:07:33Z

thanks! I've merged this @Aliraza-N

TheNeuralBit · 2020-11-30T21:57:17Z

I think this broke Java Postcommit: https://ci-beam.apache.org/job/beam_PostCommit_Java/6904/

java.nio.file.NoSuchFileException: src/test/resources/DICOM/testDicomFile.dcm
	at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
	at sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:214)
	at java.nio.file.Files.newByteChannel(Files.java:361)
	at java.nio.file.Files.newByteChannel(Files.java:407)
	at java.nio.file.Files.readAllBytes(Files.java:3152)
	at org.apache.beam.sdk.io.gcp.healthcare.HttpHealthcareApiClient.uploadToDicomStore(HttpHealthcareApiClient.java:229)
	at org.apache.beam.sdk.io.gcp.healthcare.DicomIOReadIT.setup(DicomIOReadIT.java:55)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

Aliraza-N · 2020-11-30T22:11:17Z

I think this broke Java Postcommit: https://ci-beam.apache.org/job/beam_PostCommit_Java/6904/

java.nio.file.NoSuchFileException: src/test/resources/DICOM/testDicomFile.dcm
	at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
	at sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:214)
	at java.nio.file.Files.newByteChannel(Files.java:361)
	at java.nio.file.Files.newByteChannel(Files.java:407)
	at java.nio.file.Files.readAllBytes(Files.java:3152)
	at org.apache.beam.sdk.io.gcp.healthcare.HttpHealthcareApiClient.uploadToDicomStore(HttpHealthcareApiClient.java:229)
	at org.apache.beam.sdk.io.gcp.healthcare.DicomIOReadIT.setup(DicomIOReadIT.java:55)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

Working on a fix. In the meantime, I'll add the @ Ignore tag

TheNeuralBit · 2020-11-30T22:18:11Z

Thank you!

Aliraza-N · 2020-11-30T22:34:32Z

This PR has the change needed to ignore the test. At first glance, it appears that the test input file is not available during the run.
#13440

suztomo · 2021-04-05T19:25:14Z

@Aliraza-N The DiacomIOTest and other gcp healthcare service tests require default application credentials.

There are additional tests that suffer from this. They appear to be GCP integration tests, but have Test in the class name and so run under the :test task:
DicomIOTest. test_Dicom_failedMetadataRead
FhirIOTest. test_FhirIO_failedReads
FhirIOTest. test_FhirIO_failedWrites
HL7v2IOTest. test_HL7v2IO_failedReads
HL7v2IOTest. test_HL7v2IO_failedWrites

https://issues.apache.org/jira/browse/BEAM-11363?focusedCommentId=17242773&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17242773

Exception:

Caused by: java.io.IOException: The Application Default Credentials are not available. They are available if running in Google Compute Engine. Otherwise, the environment variable GOOGLE_APPLICATION_CREDENTIALS must be defined pointing to a file defining the credentials. See https://developers.google.com/accounts/docs/application-default-credentials for more information.
	at com.google.auth.oauth2.DefaultCredentialsProvider.getDefaultCredentials(DefaultCredentialsProvider.java:134)
	at com.google.auth.oauth2.GoogleCredentials.getApplicationDefault(GoogleCredentials.java:119)
	at com.google.auth.oauth2.GoogleCredentials.getApplicationDefault(GoogleCredentials.java:91)
	at org.apache.beam.sdk.io.gcp.healthcare.HttpHealthcareApiClient.initClient(HttpHealthcareApiClient.java:691)
	at org.apache.beam.sdk.io.gcp.healthcare.HttpHealthcareApiClient.<init>(HttpHealthcareApiClient.java:121)
	at org.apache.beam.sdk.io.gcp.healthcare.DicomIO$ReadStudyMetadata$FetchStudyMetadataFn.instantiateHealthcareClient(DicomIO.java:164)

Do you think they should be classified as integration tests?

If not, is there a way to run this test in GitHub Actions?

pabloem · 2021-04-08T19:30:42Z

hmmm I think these should be run as IT tests indeed, and only whenever application default creds are available

aaltay · 2021-04-22T02:23:43Z

hmmm I think these should be run as IT tests indeed, and only whenever application default creds are available

Who could make this change?

Aliraza-N added 5 commits October 5, 2020 20:26

New Dicom IO connector

0cf68a2

Revert "New Dicom IO connector"

49cb6ef

This reverts commit 0cf68a2.

Merge remote-tracking branch 'upstream/master'

d5ddcee

Merge remote-tracking branch 'upstream/master'

02f6713

New Dicom IO connector

5437fda

poojavenkatram reviewed Oct 21, 2020

View reviewed changes

Dicom IO connector output changed to a Result class

12d2257

Aliraza-N commented Oct 22, 2020

View reviewed changes

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java Outdated Show resolved Hide resolved

Aliraza-N added 2 commits October 22, 2020 19:00

[BEAM-11073] Dicom IO connector output changed to a Result class

7748e8a

Merge branch 'master' of https://github.com/Aliraza-N/beam

c46c0c5

poojavenkatram suggested changes Oct 22, 2020

View reviewed changes

Aliraza-N added 2 commits October 23, 2020 19:43

Renamed tuple tags in DicomIO

1e2dcb4

[BEAM-11073] DicomIO - adjusted failure test case

85600b7

Aliraza-N commented Oct 26, 2020

View reviewed changes

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java Outdated Show resolved Hide resolved

poojavenkatram suggested changes Oct 26, 2020

View reviewed changes

Made changes to the documentation for DicomIO

d6b1172

poojavenkatram approved these changes Oct 26, 2020

View reviewed changes

[BEAM-11073] DicomIO documentation fixes

af24750

pabloem self-requested a review November 3, 2020 20:39

pabloem reviewed Nov 3, 2020

View reviewed changes

DicomIO - New docs + connector requires a String input

66e5640

pabloem reviewed Nov 5, 2020

View reviewed changes

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java Outdated Show resolved Hide resolved

DicomIO Integration test

7bcbfa3

pabloem reviewed Nov 12, 2020

View reviewed changes

...google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIOReadIT.java Show resolved Hide resolved

pabloem reviewed Nov 12, 2020

View reviewed changes

...va/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java Outdated Show resolved Hide resolved

pabloem reviewed Nov 12, 2020

View reviewed changes

Aliraza-N added 2 commits November 12, 2020 22:35

DicomIO - updated integration test + small fixes

6c4a87f

DicomIO - added null checks after supressing warnings

f923e67

pabloem reviewed Nov 13, 2020

View reviewed changes

...google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIOReadIT.java Show resolved Hide resolved

Dicom IO - more checks in IT

9b59713

GCP healthcare IO - new utility class for parsing webpaths

28ef111

probot-autolabeler bot added gcp io java labels Nov 26, 2020

GCP Webpath parser - handle null warnings

63e22da

Dicom API method uses Webpath parser

069a4e8

pabloem merged commit 978b812 into apache:master Nov 30, 2020

Aliraza-N mentioned this pull request Dec 1, 2020

[BEAM-11376] Ignoring DicomIOReadIT #13450

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-11073] Dicom IO Connector for Java #13137

[BEAM-11073] Dicom IO Connector for Java #13137

Aliraza-N commented Oct 19, 2020 •

edited

Aliraza-N commented Oct 19, 2020

poojavenkatram left a comment

poojavenkatram left a comment

poojavenkatram left a comment

poojavenkatram left a comment

Aliraza-N commented Oct 26, 2020

pabloem commented Nov 3, 2020

pabloem left a comment

pabloem commented Nov 5, 2020

Aliraza-N commented Nov 6, 2020

Aliraza-N commented Nov 6, 2020

Aliraza-N commented Nov 9, 2020

pabloem Nov 12, 2020

pabloem Nov 12, 2020

Aliraza-N Nov 12, 2020

pabloem left a comment

Aliraza-N commented Nov 13, 2020

pabloem commented Nov 13, 2020

Aliraza-N commented Nov 13, 2020 •

edited

aaltay commented Nov 25, 2020

Aliraza-N commented Nov 26, 2020

Aliraza-N commented Nov 26, 2020

pabloem commented Nov 30, 2020

TheNeuralBit commented Nov 30, 2020

Aliraza-N commented Nov 30, 2020 •

edited

TheNeuralBit commented Nov 30, 2020

Aliraza-N commented Nov 30, 2020

suztomo commented Apr 5, 2021 •

edited

pabloem commented Apr 8, 2021

aaltay commented Apr 22, 2021

	private HealthcareApiClient dicomStore;
	private HealthcareApiClient dicomStore = new HttpHealthcareApiClient();

[BEAM-11073] Dicom IO Connector for Java #13137

[BEAM-11073] Dicom IO Connector for Java #13137

Conversation

Aliraza-N commented Oct 19, 2020 • edited

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Aliraza-N commented Oct 19, 2020

poojavenkatram left a comment

Choose a reason for hiding this comment

poojavenkatram left a comment

Choose a reason for hiding this comment

poojavenkatram left a comment

Choose a reason for hiding this comment

poojavenkatram left a comment

Choose a reason for hiding this comment

Aliraza-N commented Oct 26, 2020

pabloem commented Nov 3, 2020

pabloem left a comment

Choose a reason for hiding this comment

pabloem commented Nov 5, 2020

Aliraza-N commented Nov 6, 2020

Aliraza-N commented Nov 6, 2020

Aliraza-N commented Nov 9, 2020

pabloem Nov 12, 2020

Choose a reason for hiding this comment

pabloem Nov 12, 2020

Choose a reason for hiding this comment

Aliraza-N Nov 12, 2020

Choose a reason for hiding this comment

pabloem left a comment

Choose a reason for hiding this comment

Aliraza-N commented Nov 13, 2020

pabloem commented Nov 13, 2020

Aliraza-N commented Nov 13, 2020 • edited

aaltay commented Nov 25, 2020

Aliraza-N commented Nov 26, 2020

Aliraza-N commented Nov 26, 2020

pabloem commented Nov 30, 2020

TheNeuralBit commented Nov 30, 2020

Aliraza-N commented Nov 30, 2020 • edited

TheNeuralBit commented Nov 30, 2020

Aliraza-N commented Nov 30, 2020

suztomo commented Apr 5, 2021 • edited

pabloem commented Apr 8, 2021

aaltay commented Apr 22, 2021

Aliraza-N commented Oct 19, 2020 •

edited

Aliraza-N commented Nov 13, 2020 •

edited

Aliraza-N commented Nov 30, 2020 •

edited

suztomo commented Apr 5, 2021 •

edited