DICOMweb URL manipulation for Google Cloud Healthcare API

[agharwal]

Markus - thank you for your kind consideration!

The PR introduces a GoogleCloudHealthcareURL class to facilitate parsing and creation of DICOMweb API Service URLs for the Google Cloud Healthcare API.

Implementation notes:

• Since the scope of the helper methods is restricted to the base_url parameter, I focused the API's interactions with the base_url string.
• To facilitate reversible transformations (str <--> CHC DICOM Store), more than one method was needed. In order to limit the footprint of vendor-specific features within the uri module, I opted to encapsulate the requisite functionality under a dedicated class.
• And finally, in addition to transformations, users may also want to store the Resource IDs parsed from the CHC endpoint URL. An attribute container seemed like a natural candidate to capture all the requirements. The implementation subclasses attr in favor of dataclasses.dataclass for compatibility with Python 3.6. I understand, however, that this is not part of the Standard Library, so you may prefer dataclass with a Python 3.6 backport. Alternately, we could drop any such dependency and implement a custom container. Given how simple the non-attribute-functionality is, however, this may be overkill.

I look forward to your feedback :-)

[agharwal]

I see that this breaks the build for Python versions 3.6 and 3.7 due to missing features of attr. Once we converge on the implementation approach (attr, dataclasses, custom container, or potentially something else), I'll go ahead and fix if still needed.

[pieper]

I'm going to jump in here and suggest that Google support doesn't should not be a first class property of the python package, but should be provided as another layer on top.

I added support to Slicer that assumes gcloud is installed on the system and it would be nice to do those transactions in native python with your code, but I wouldn't want to put vendor-specific support in the client code. It's just a matter of finding the dicom store, getting the token, and passing it in as a header. So the client code can be complete generic and standards-based.

We also follow this model in javascript, where the dicomweb client is a layer beneath the Google-specific project/location/dataset/dicomstore layers.

[hackermd]

I wouldn't want to put vendor-specific support in the client code.

I generally agree with you @pieper and share your concerns regarding the inclusion of code that is specific to a particular vendor implementation.
At the same time, I see value in making it easy for users of the library to connect to the Google Cloud Healthcare API and we already have GCP-specific code in the library (see session_utils.create_session_from_gcp_credentials()). OAuth 2.0 authorization flows are much more challenging for users to implement and are deeper integrated into the library code (access token refreshing, etc.) than the construction of a URL, which can really just be a thin layer on top. I am ambivalent and see risks and benefits.

[hackermd]

@ntenenz what do you think?

[ntenenz]

@ntenenz what do you think?

I agree with @pieper that having this functionality in the main client feels a bit strange, in particular, because it adds a fairly minor feature (string templatization) in exchange for what appears to be an added dependency. An alternate approach may be to conditionally make available vendor-specific clients with auth, path building, etc built-in if dicomweb-client[vendor] is installed and then namespace them accordingly, eg dicomweb_client.ext.vendor. This all assumes you're comfortable with the added maintenance of course.

As an aside, I don't see the attr dependency declared in the package dependencies.

[hackermd]

Thanks for feedback @ntenenz. I like the idea and would like to explore it further. We already have gcp as a extra requirement (see setup.py) and could build on that.

[hackermd]

@agharwal I have put further thought into this PR and @ntenenz's suggestion. I like the idea of creating vendor-specific extensions (dicomweb_client.ext.{vendor}). We could either create the subpackage in the same repository using extra requirements or even in a separate repository by creating a Python namespace package.

@ntenenz @agharwal what do you think?

[ntenenz]

I'd recommend using extra requirements. Relative to the size of the added code, I wouldn't want to handle the extra complexity and package management that comes along with namespace packages.

[hackermd]

I'd recommend using extra requirements. Relative to the size of the added code, I wouldn't want to handle the extra complexity and package management that comes along with namespace packages.

Sounds good. That's also the approach we have been taking so far. Implementing the extension mechanism will require minor changes to the repository structure.

@agharwal I would suggest moving the GoogleCloudHealthcareURL class into dicomweb_client.ext.gcp.uri.

I would further suggest moving the create_session_from_gcp_credentials function from dicomweb_client.session_utils to dicomweb_client.ext.gcp.session_utils (keeping dicomweb_client.session_utils.create_session_from_gcp_credentials as a wrapper that calls dicomweb_client.ext.gcp.session_utils.create_session_from_gcp_credentials internally and issues a DeprecationWarning).

[ntenenz]

Given the move, this may be a good time to revisit the API/implementation. If the GCP functionality can be accomplished with a decorator, it may make sense to first implement the proposal discussed in #50 and then create a GCPDICOMwebClient which injects the required decorator.

[hackermd]

If the GCP functionality can be accomplished with a decorator, it may make sense to first implement the proposal discussed in #50 and then create a GCPDICOMwebClient which injects the required decorator.

What exactly do you have in mind? I don't think the extension mechanism would require passing any decorators to DICOMwebClient to wrap HTTP request calls and I thus think we could pursue both efforts in parallel. Am I missing something?

[ntenenz]

I'm unfamiliar with the Google product, however if it requires custom headers or auth, the decorator approach would be a pretty seamless way of incorporating it. I'm assuming the vendor-specific client would wrap all vendor functionality under a single umbrella for ease of use?

[agharwal]

Sounds good. That's also the approach we have been taking so far. Implementing the extension mechanism will require minor changes to the repository structure.

@agharwal I would suggest moving the GoogleCloudHealthcareURL class into dicomweb_client.ext.gcp.uri.

I would further suggest moving the create_session_from_gcp_credentials function from dicomweb_client.session_utils to dicomweb_client.ext.gcp.session_utils (keeping dicomweb_client.session_utils.create_session_from_gcp_credentials as a wrapper that calls dicomweb_client.ext.gcp.session_utils.create_session_from_gcp_credentials internally and issues a DeprecationWarning).

@hackermd: Apologies for the delayed turnaround, and thank you for the feedback! Per your suggestion, I've moved uri.GoogleCloudHealthcareURL to dicomweb_client.ext.gcp.uri and session_utils.create_session_from_gcp_credentials() to dicomweb_client.ext.gcp.session_utils. Additionally, I've changed the base class of GoogleCloudHealthcareURL to dataclasses.dataclass from attrs to avoid introducing a dependency outside the Python Standard Library for versions >3.6. Please let me know if this seems reasonable.

@ntenenz: Thank you for helping incorporate vendor-specific implementations in a meaningful and non-disruptive fashion.

[ntenenz]

Glad to see the project is still going strong.

[hackermd]

@agharwal this looks great! Thank you! I have only one minor suggestion/question.