Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MONAI Deploy Application Server #9

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

pritishnahar95
Copy link
Contributor

@pritishnahar95 pritishnahar95 commented Jun 21, 2021

This PR adds a draft of the MONAI Deploy App Server specification.

This contains a description of the initial set of requirements and design for MONAI Deploy App Server.

@pritishnahar95 pritishnahar95 force-pushed the pnahar/app-server-requirements branch from e66a199 to efe7df1 Compare June 21, 2021 17:20
guidelines/MONAI-App-Server.md Outdated Show resolved Hide resolved
guidelines/MONAI-App-Server.md Outdated Show resolved Hide resolved
@whoisj whoisj linked an issue Jun 21, 2021 that may be closed by this pull request
@whoisj
Copy link
Collaborator

whoisj commented Jun 21, 2021

/CC @KarthikMasi @bhatt-piyush to review and provide feedback.

@pritishnahar95 pritishnahar95 force-pushed the pnahar/app-server-requirements branch from efe7df1 to 9235a2a Compare June 21, 2021 17:59
@pritishnahar95 pritishnahar95 force-pushed the pnahar/app-server-requirements branch 3 times, most recently from b8dafaf to 67314a8 Compare June 21, 2021 19:10
guidelines/MONAI-App-Server.md Outdated Show resolved Hide resolved
guidelines/MONAI-App-Server.md Outdated Show resolved Hide resolved
guidelines/MONAI-App-Server.md Outdated Show resolved Hide resolved
guidelines/MONAI-App-Server.md Outdated Show resolved Hide resolved
@ristoh
Copy link
Collaborator

ristoh commented Jun 22, 2021

This PR adds a draft of the MONAI Deploy App Server specification.

This contains a description of the initial set of requirements for MONAI Deploy App Server.

This PR does not include any design or implementation. These will be made available in future PRs.

Similar comments for this as for the MAP PR #7 :

We need to decide if this is a Guideline or Feature, in case this is a feature I'll propose the following changes on this:

change the branch name to be:

  • feature/4-app-server
  • make the feature specification available in folder features/app-server.md

cc @hshuaib90

@ristoh ristoh requested a review from hshuaib90 June 22, 2021 01:46
@hshuaib90
Copy link
Collaborator

This PR adds a draft of the MONAI Deploy App Server specification.
This contains a description of the initial set of requirements for MONAI Deploy App Server.
This PR does not include any design or implementation. These will be made available in future PRs.

Similar comments for this as for the MAP PR #7 :

We need to decide if this is a Guideline or Feature, in case this is a feature I'll propose the following changes on this:

change the branch name to be:

  • feature/4-app-server
  • make the feature specification available in folder features/app-server.md

cc @hshuaib90

Similar to comment I made for #7, the primary output of this PR is a document rather than code and as such it should be classified as Guideline.

@pritishnahar95 pritishnahar95 force-pushed the pnahar/app-server-requirements branch from 67314a8 to ec60323 Compare June 22, 2021 17:47
@pritishnahar95 pritishnahar95 changed the title MONAI Deploy App Server Requirements MONAI Deploy Application Server Requirements Jun 22, 2021
@whoisj
Copy link
Collaborator

whoisj commented Jun 22, 2021

@ristoh @hshuaib90 please see my comment on #7 as well. I think we're heading down the wrong path here.

@whoisj whoisj changed the title MONAI Deploy Application Server Requirements MONAI Deploy Application Server Jun 22, 2021
@whoisj whoisj added the guideline MONAI Deploy guidelines and design label Jun 22, 2021
Copy link
Collaborator

@hshuaib90 hshuaib90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These requirements are a good start but I do not think they cover the requirements we have already discussed in our WG meetings. I think we should at least cover those in 0.1 release.

I would also propose amending the document name to MONAI-App-Server-Requirements.md

guidelines/MONAI-App-Server.md Outdated Show resolved Hide resolved
guidelines/MONAI-App-Server.md Outdated Show resolved Hide resolved
@whoisj
Copy link
Collaborator

whoisj commented Jun 23, 2021

@hshuaib90 can you expand on your statement?

These requirements are a good start but I do not think they cover the requirements we have already discussed in our WG meetings.

@hshuaib90
Copy link
Collaborator

@hshuaib90 can you expand on your statement?

These requirements are a good start but I do not think they cover the requirements we have already discussed in our WG meetings.

For example, we have discussed the requirement to have the ability to execute nondeterministic chains or DAGs made up of individual applications, as well was things like facilitating the sending of results to clinical information systems such as Picture Archiving and Communication Systems. There is a helpful diagram that Jorge added here: https://docs.google.com/document/d/1fzG3z7TxB9SzWdfqsApAMFrM91nHfYiISnSz4QHJHrM/edit#heading=h.677zfo5wg7au

@whoisj
Copy link
Collaborator

whoisj commented Jun 23, 2021

For example, we have discussed the requirement to have the ability to execute nondeterministic chains or DAGs made up of individual applications

While that has been a discussion point, I feel it is well outside the scope of the initial version of the Application Server. Functionality such as this would require a much more elaborate specification, design, and investment than we can likely make with the initial version.

as well was things like facilitating the sending of results to clinical information systems such as Picture Archiving and Communication Systems.

Again, outside the scope of the Application Server. The goal of the Application Server is to serve applications on demand based on requests from external entities (PACS, users, etc.). What you're describing here sounds very much like the "infomatics gateway" @rahul-imaging describes, which is a separate service which sits along side the Application Server.

Please let me know if I am making sense, or (likely) not quite understanding something and missing the point. 🙂

@pritishnahar95 pritishnahar95 force-pushed the pnahar/app-server-requirements branch 4 times, most recently from d05c661 to 484a123 Compare July 20, 2021 22:20
@whoisj
Copy link
Collaborator

whoisj commented Jul 21, 2021

I like the diagrams you've added, but as a point of discussion with the WG in general: do we prefer to see sequence diagrams in the format currently in the PR, or would we prefer more traditional sequence diagrams. Examples below:

sequence diagram describing the MONAI Deploy Server version zero job workflow

sequence diagram describing the MONAI Deploy Server version zero map registration workflow

@pritishnahar95 pritishnahar95 force-pushed the pnahar/app-server-requirements branch 7 times, most recently from 68ca956 to dd2ff86 Compare July 30, 2021 19:43
Copy link
Collaborator

@whoisj whoisj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed via Teams a few select changes, mostly around making everything into bullet points or numbered lists. Assuming those changes are made, I'm good with this.

@pritishnahar95 pritishnahar95 force-pushed the pnahar/app-server-requirements branch from dd2ff86 to 8d2b4be Compare July 31, 2021 00:14
guidelines/application-server.md Outdated Show resolved Hide resolved
guidelines/application-server.md Outdated Show resolved Hide resolved
guidelines/application-server.md Outdated Show resolved Hide resolved
guidelines/application-server.md Outdated Show resolved Hide resolved
guidelines/application-server.md Show resolved Hide resolved
guidelines/application-server.md Show resolved Hide resolved
guidelines/application-server.md Outdated Show resolved Hide resolved
guidelines/application-server.md Show resolved Hide resolved
guidelines/application-server.md Outdated Show resolved Hide resolved
guidelines/application-server.md Outdated Show resolved Hide resolved
@pritishnahar95 pritishnahar95 force-pushed the pnahar/app-server-requirements branch 5 times, most recently from a5247f6 to 205ee4e Compare August 4, 2021 18:58
@pritishnahar95 pritishnahar95 force-pushed the pnahar/app-server-requirements branch from 205ee4e to 259af73 Compare August 5, 2021 00:14
@pritishnahar95 pritishnahar95 merged commit 0213831 into main Aug 6, 2021
@pritishnahar95 pritishnahar95 deleted the pnahar/app-server-requirements branch August 6, 2021 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guideline MONAI Deploy guidelines and design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MVP of MONAI Application Server