User Guide #15

mocsharp · 2021-09-20T23:37:08Z

Description

Add user guide skeleton based on docfx format from donet.

include .NET APIs in user guide
include markdown in user guide

Status

Ready/Work in progress/Hold

Types of changes

Non-breaking change (fix or new feature that would not break existing functionality).
Breaking change (fix or new feature that would cause existing functionality to change).
New tests added to cover the changes.
All tests passed locally by running ./src/run-tests-in-docker.sh.
Documentation comments included/updated.
User guide updated.
I have updated the changelog

codecov · 2021-09-20T23:57:52Z

Codecov Report

Merging #15 (1aeb731) into main (9f7aec0) will increase coverage by 0.07878%.
The diff coverage is 100.00000%.

@@ Coverage Diff @@ ## main #15 +/- ## =================================================== + Coverage 79.72474% 79.80352% +0.07878%  =================================================== Files 142 142 Lines 7847 7838 -9 Branches 555 555 =================================================== - Hits 6256 6255 -1  + Misses 1398 1389 -9  - Partials 193 194 +1

Flag	Coverage Δ
unittests	`79.80352% <100.00000%> (+0.07878%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/Api/Rest/InferenceRequestException.cs	`100.00000% <ø> (+75.00000%)`	⬆️
src/DicomWebClient/DicomWebClient.cs	`97.43590% <ø> (ø)`
src/DicomWebClient/Services/WadoService.cs	`80.00000% <ø> (ø)`
...maticsGateway/Services/Http/InferenceController.cs	`93.10345% <ø> (ø)`
src/Client/InformaticsGatewayClient.cs	`100.00000% <100.00000%> (ø)`
...Gateway/Repositories/InferenceRequestRepository.cs	`93.69369% <100.00000%> (ø)`
...eway/Services/Http/DestinationAeTitleController.cs	`88.40580% <100.00000%> (ø)`
...formaticsGateway/Services/Http/HealthController.cs	`93.75000% <100.00000%> (ø)`
...icsGateway/Services/Http/MonaiAeTitleController.cs	`88.75000% <100.00000%> (ø)`
...csGateway/Services/Http/SourceAeTitleController.cs	`88.57143% <100.00000%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f7aec0...1aeb731. Read the comment docs.

jonanswer · 2021-09-29T07:55:58Z

docs/api/rest/config.md

+
+The _configuration_ endpoint provide the following APIs to configured the Informatics Gateway.
+
+## GET /config/monaiaetitle


would it make the API easier to read if we used separators in the words? e.g:

GET /config/monai-ae-title

I'll change the following:
/config/monaiaetitle => /config/ae
/config/sourceaetitle => /config/source
/config/destinationaetitle => /config/destination

It is better to have consistency: the first config item is names ae, and the next two are also aet which should also have the word aet.
Also, the source is really the calling AET, and the ae really is called AET. It is better to be consistent with the commonly used terms. The destination is really the store AET

Also, the line below states SCP, but that destination or store AET does not belong to the SCP of the IG.

@jonanswer, @GreySeaWolf any suggestions here?

Option 1

/config/aet /config/source /config/destination

Option 2

/config/called-aet /config/calling-aet /config/store-aet

See definition here in this commercial DICOM conformance doc; the terms are based on industry convention, https://www.medical.canon/Interoperability/dicom/miict0021ea.pdf

However, I'd change my opinion on the store-aet, to results-aet (ACR calls Results Repo), which is more logically correct for what we intend to use for the external called AET for reporting AI results in DICOM instances.

docs/api/rest/config.md

MMelQin · 2021-10-05T20:29:45Z

docs/api/rest/config.md

+ {
+ "name": "brain-tummor",
+ "aeTitle": "BrainTumorModel",
+ "applications": ["brain-tumor", "b75cd27a-068a-4f9c-b3da-e5d4ea08c55a"]


Why is the mapping to the applications or pipelines in the IG?

Since we are allowing AET-to-application mapping for the first release, we allow users to include this optional property in the request payload.

This also allows ACR API call to include the application ID/name to bypass DDS.

What has the ACR like API to do with IG? The ACR API is a first class AI inference RESTFul API, and as I mentioned, the URL should have the model/app embedded as the last fragment, as ACR defined it. Some implementations lack the ability to do reverse proxy so the model/app name is embedded in the message content.

The proper design should be,
IG records the called AET, stores the data, notification raised so the Workload Manager parses the message, based on the called AET, the WM then matches the payload to model/app.

That is the reason I specifically called for the separate entity WM.

I am concern about usability:
If the user configures applications on the IG side, there is a single command to run to complete the setup. On the other hand, if we pass AET to WM, that means the user would have to configure that aet-to-app mapping on the WM side. 2 commands to complete the setup. (one to set up AET and one to set up app).

Also, with ACR API call, it's the application that is passed in as part of the payload since there is no AET. IMO, it's better to have fewer APIs and reduce the number of steps required for sysadmins to set up. This would also reduce the misconfigurations.

I'd think good usability comes naturally with a design that has clear responsibility of each service/component:

The ACR like RESTFul API should not even been in the context of the Informatics Gate, for which we have clearly defined the responsibilities. The ACR API is meant to be a inference API whose URL identifies the model/app (or the app name moved into the message body where multiple endpoints might not be easily deployed, like in Clara), and the message content defines the full scope of the input data (just happens that the resource may just be DICOM instance UIDs). This API should never have been mixed with a data ingestion API; it is better not to drag the design deficiency into MONAI Deploy.

The Informatics Gateway, like many of the DICOM ingestion/PACS implementation, simply receives and stores DICOM instances while recording the called and calling AET, then registers the series/instances, which may trigger event/message so that the interested services/components can act with its own logic, e.g. the Workload Manager. Note, this is how MergeCom3 was used in my previous commercial PACS, and Answer Digital is also using a similar design approach.

Now on to the message/event handling. There is a construct/attributes for each message providing a hint to the associated application, e.g. called aet for a received study, application name for the retrieved series/instance from the ACR like API, so that the Workflow Manager can use applicable logic to dispatch the study/series as payload for the resolved the application(s). When there is no hint, (typically there would be a "default" aet on the DICOM SCP), then there need to be heuristics to try to figure out what study for what applications, e.g. using metadata and rules or even image processing algorithms.

This is the design that I had drafted up for the DICOM Ingestion (IG), simple Storage Service (not long-term archive as required by a PACS, but long enough for possible re-run and re-use), and the Workload Manager. If still wanting to have the IG to perform the roles of both DICOM Ingestion and Workload Manager, for the initial release, certain separation of concerns and responsibilities still need to be maintained, so that the components can be cleanly and simply partitioned out in later releases.

All good points. Originally I wanted to put ACR part of WM but due to time+resource constraints, I just left it as-is.

Docker support - CLI to support start/stop/restart with Docker

Add user guide

e911092

mocsharp changed the title ~~Vchang/docs~~ User Guide Sep 20, 2021

Update SADD to reference user guide

494a81a

mocsharp changed the base branch from main to vchang/cli September 20, 2021 23:55

mocsharp marked this pull request as ready for review September 20, 2021 23:56

mocsharp self-assigned this Sep 20, 2021

mocsharp added the documentation Improvements or additions to documentation label Sep 20, 2021

Update unit test

8701720

Base automatically changed from vchang/cli to main September 22, 2021 15:49

jonanswer reviewed Sep 29, 2021

View reviewed changes

docs/api/rest/config.md Outdated Show resolved Hide resolved

Update route for ae configurations

1aeb731

MMelQin reviewed Oct 5, 2021

View reviewed changes

mocsharp requested a review from GreySeaWolf October 5, 2021 21:54

mocsharp and others added 3 commits October 6, 2021 09:50

Docker support for CLI start/stop/restart

c779f86

Update unit tests

7081f49

Merge pull request #17 from Project-MONAI/vchang/cli_docker

380d8ee

Docker support - CLI to support start/stop/restart with Docker

mocsharp merged commit 062d573 into main Nov 15, 2021

mocsharp deleted the vchang/docs branch February 3, 2022 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

User Guide #15

User Guide #15

Uh oh!

mocsharp commented Sep 20, 2021 •

edited

Loading

codecov bot commented Sep 20, 2021 •

edited

Loading

jonanswer Sep 29, 2021

mocsharp Sep 29, 2021 •

edited

Loading

MMelQin Oct 5, 2021

MMelQin Oct 5, 2021

mocsharp Oct 5, 2021 •

edited

Loading

MMelQin Oct 5, 2021

Uh oh!

MMelQin Oct 5, 2021

mocsharp Oct 5, 2021

MMelQin Oct 5, 2021 •

edited

Loading

MMelQin Oct 5, 2021

mocsharp Oct 5, 2021 •

edited

Loading

MMelQin Oct 6, 2021

mocsharp Oct 6, 2021

Labels

4 participants


		The _configuration_ endpoint provide the following APIs to configured the Informatics Gateway.

		## GET /config/monaiaetitle

User Guide #15

User Guide #15

Uh oh!

Conversation

mocsharp commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Types of changes

codecov bot commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Choose a reason for hiding this comment

GET /config/monai-ae-title

mocsharp Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mocsharp Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMelQin Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mocsharp Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

4 participants

mocsharp commented Sep 20, 2021 •

edited

Loading

codecov bot commented Sep 20, 2021 •

edited

Loading

mocsharp Sep 29, 2021 •

edited

Loading

mocsharp Oct 5, 2021 •

edited

Loading

MMelQin Oct 5, 2021 •

edited

Loading

mocsharp Oct 5, 2021 •

edited

Loading