healthcare API: update all DICOM v1beta1 samples to v1 #3387

noerog · 2020-04-13T22:15:36Z

Originally from #3300 but split up here

* healthcare API: update all v1beta1 samples to v1 * healthcare API: use v1 instead of v1beta1 endpoint * healthcare API: remove FHIR conditional methods from v1 because they are only in v1beta1 * add requirements-test.txt files * Change test response for searching for studies to 204 * fix missing param in execute_bundle.json causing test failures * fix HL7v2 store failure. notificationConfig was deprecated in favor of notificationConfigs * address review comments: use ADC and simplify credentials auth * delete new FHIR files, they are now in #3384 * delete new DICOM files, they are now in #3387 * delete new HL7v2 files, they are now in #3388 * clean up test_dataset fixture * remove try block from the sample * more robust test setup and cleanup Co-authored-by: Takashi Matsuo <[email protected]>

tmatsuo

You may want to do some research on whether we still need requests before actually make changes.

healthcare/api-client/v1/dicom/dicom_stores.py

healthcare/api-client/v1/dicom/dicomweb.py

tmatsuo · 2020-04-15T21:59:19Z

healthcare/api-client/v1/dicom/dicomweb.py

+    content_type = 'application/dicom'
+    headers = {'Content-Type': content_type}
+
+    try:


Can you remove try-except?

Seems like it reverted and it still has try-except.

healthcare/api-client/v1/dicom/dicomweb_test.py

tmatsuo · 2020-04-20T17:06:53Z

healthcare/api-client/v1/dicom/dicom_stores.py

+    service_name = "healthcare"
+
+    return discovery.build(service_name, api_version)
+


Nit: can you remove 2 empty lines after the return statement and [END healthcare_get_client]?
Also here and there.

I would but that's an effect of running the black tool that we're supposed to be using to lint everything. I'd have to manually go back and remove the lines every time I run the lint tool.

@kurtisvg Is there a way to instruct black not inserting the empty lines?

If you indent the comment to match the function line, Black will likely treat it as part of the function and won't add the extra lines (otherwise I believe it's a PEP8 rule to have 2 empty lines after a top level function).

That's not ideal because that goes against style for writing comments, no?

tmatsuo · 2020-04-20T17:10:20Z

healthcare/api-client/v1/dicom/dicomweb.py

+    content_type = 'application/dicom'
+    headers = {'Content-Type': content_type}
+
+    try:


Seems like it reverted and it still has try-except.

tmatsuo · 2020-04-20T17:14:55Z

healthcare/api-client/v1/dicom/dicomweb_test.py

+    assert "Retrieved study" in out
+
+    # Delete downloaded study
+    os.remove("study.multipart")


Not a big deal, but can you put the os.remove() call in finally clause?

try: ... ... finally: os.remove("study.multipart")

also for some tests below

kurtisvg · 2020-04-21T17:46:50Z

healthcare/api-client/v1/dicom/dicom_stores.py

+    service_name = "healthcare"
+
+    return discovery.build(service_name, api_version)
+


If you indent the comment to match the function line, Black will likely treat it as part of the function and won't add the extra lines (otherwise I believe it's a PEP8 rule to have 2 empty lines after a top level function).

healthcare/api-client/v1/dicom/dicom_stores_test.py

kurtisvg · 2020-04-21T17:50:16Z

healthcare/api-client/v1/dicom/dicom_stores_test.py

+            datasets.delete_dataset(project_id, cloud_region, dataset_id)
+        except HttpError as err:
+            # The API returns 403 when the dataset doesn't exist.
+            if err.resp.status == 404 or err.resp.status == 403:


What about 404?

Can we update the comment to include why we are suppressing. Why do we want to surpress when it doesn't exist?

It's a 403 when calling delete on a dataset that doesn't exist. Calling delete on a DICOM store that doesn't exist returns 404. Calling delete on a DICOM store that's in a dataset when the dataset doesn't exist returns a 403.

We can suppress it because if the dataset doesn't exist then we don't need to delete it.

Can we add that to the comment :) (L85-87 is a good example)

healthcare/api-client/v1/dicom/dicomweb.py

kurtisvg

One nit then LGTM.

kurtisvg · 2020-04-21T19:25:26Z

healthcare/api-client/v1/dicom/dicom_stores_test.py

+            datasets.delete_dataset(project_id, cloud_region, dataset_id)
+        except HttpError as err:
+            # The API returns 403 when the dataset doesn't exist.
+            if err.resp.status == 404 or err.resp.status == 403:


Can we add that to the comment :) (L85-87 is a good example)

…ests

noerog · 2020-04-21T19:55:34Z

One nit then LGTM.

Sure, I actually expanded the comment on deleting the DICOM store. The dataset behavior is straightforward--if it doesn't exist, server returns 403.

noerog requested a review from a team as a code owner April 13, 2020 22:15

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 13, 2020

blunderbuss-gcf bot assigned tmatsuo Apr 13, 2020

noerog mentioned this pull request Apr 13, 2020

healthcare API: update v1beta1 datasets samples to v1 #3300

Merged

tmatsuo suggested changes Apr 15, 2020

View reviewed changes

tmatsuo reviewed Apr 20, 2020

View reviewed changes

tmatsuo approved these changes Apr 20, 2020

View reviewed changes

noerog requested a review from kurtisvg April 21, 2020 00:00

kurtisvg suggested changes Apr 21, 2020

View reviewed changes

noerog added 7 commits April 21, 2020 15:21

healthcare API: update all DICOM v1beta1 samples to v1

5e15796

add DICOM test file

688e735

make DICOM v1 samples consistent with other modalities

d5319f5

remove HttpError import

26c9dd8

shorten docstring

32cc36f

address review comments

9a85a68

fix nits

9914bb8

kurtisvg approved these changes Apr 21, 2020

View reviewed changes

additional context on 403/404 exceptions when deleting resources in t…

b6eefe5

…ests

Merge branch 'master' into chc-api-dicom

9825eb5

noerog merged commit 002c18a into GoogleCloudPlatform:master Apr 21, 2020

		service_name = "healthcare"

		return discovery.build(service_name, api_version)

healthcare API: update all DICOM v1beta1 samples to v1 #3387

healthcare API: update all DICOM v1beta1 samples to v1 #3387

Uh oh!

Conversation

noerog commented Apr 13, 2020

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kurtisvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noerog commented Apr 21, 2020

Uh oh!

Uh oh!