Project-MONAI / monai-deploy

MONAI Deploy aims to become the de-facto standard for developing, packaging, testing, deploying and running medical AI applications in clinical production.
Apache License 2.0
98 stars 22 forks source link

MAP v1.0 Update with Holoscan support #131

Closed mocsharp closed 1 year ago

mocsharp commented 1 year ago

This PR promotes MONAI Application Package specification to version 1.0 with support for the upcoming release of MONAI Deploy App SDK based on Holoscan SDK.

MMelQin commented 1 year ago

I find reviewing the doc change with display the rich diff option much easier, https://github.com/Project-MONAI/monai-deploy/pull/131/files?short_path=59d6146#diff-59d6146f21b5f517be4e54a472a9951c90a1ed37ae8644d8f6de33f2564a3d3b

JHancox commented 1 year ago

[like] Jonny Hancox reacted to your message:


From: Ming M Qin @.> Sent: Thursday, May 18, 2023 6:50:03 AM To: Project-MONAI/monai-deploy @.> Cc: Jonny Hancox @.>; Review requested @.> Subject: Re: [Project-MONAI/monai-deploy] MAP v1.0 Update with Holoscan support (PR #131)

I find reviewing the doc change with display the rich diff option much easier, https://github.com/Project-MONAI/monai-deploy/pull/131/files?short_path=59d6146#diff-59d6146f21b5f517be4e54a472a9951c90a1ed37ae8644d8f6de33f2564a3d3b

— Reply to this email directly, view it on GitHubhttps://github.com/Project-MONAI/monai-deploy/pull/131#issuecomment-1552567749, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ALR3LRZZSTDV5FGIVZJSW2TXGXBBXANCNFSM6AAAAAAYC5HTKU. You are receiving this because your review was requested.Message ID: @.***>

mocsharp commented 1 year ago

Do we need to specify any logging behaviour - e.g. modes such as INFO, WARN, ERROR and where and how this data should be issued? (Forgive me if I missed this!)

Are you suggesting that we add an environment variable to where the logs shall be saved so the hosting environment can map that volume for log collection? Otherwise, application logging requirements are outside the scope of MAP.

JHancox commented 1 year ago

Yes, I think mapping a volume for logging would probably suffice, but I think there should be a standard way that app logs can be collected/monitored by the coordinating system. If we already have a way of doing this then you can ignore.

mocsharp commented 1 year ago

Yes, I think mapping a volume for logging would probably suffice, but I think there should be a standard way that app logs can be collected/monitored by the coordinating system. If we already have a way of doing this then you can ignore.

I have added HOLOSCAN_LOGS to Table of Environment Variables section:

| `HOLOSCAN_LOGS`              | `/var/holoscan/logs`       | Folder Path | Path to the Application's logs.                                       |
ericspod commented 1 year ago

I had a few very minor comments and suggestions. One suggestion that is perhaps a lot of work to propose is a table to definitions for terms like "container" or "pathname" so that it's precise what we're referring to, and so that we can link to external definitions only once.

vikashg commented 1 year ago

This looks good to me @mocsharp @dbericat; based on my understanding.