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

[WIP] 486 Add example model package #487

Closed
wants to merge 22 commits into from
Closed

Conversation

Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Dec 16, 2021

Fixes #486 .

Description

This PR implemented a draft example of the model package for discussion, leverage @ericspod 's great work to save data: Project-MONAI/MONAI#3138

  1. Basic principles
    (1) Structured sharable data: Define meta data and components in structured files, like JSON or YAML. With predefined schema to verify the JSON configs, follow: https://json-schema.org/. We can put the base common schema in some public web storage in the future.
    (2) TorchScript package: Export model weights, meta data, components config, etc. into TorchScript file.
    (3) Hybrid programming: For 90% cases, only the JSON configs should be enough. For some advanced cases, the developer of model package defines which parts of the workflow should be shared for others to reconstruct other logic, then define these parts in JSON or YAML config, and can implement other customized logic in the python program. For example, this PR defines transforms, dataset, dataloader, inferer, etc. in the inference.json config, implements a task-specific special inference logic with native PyTorch program in inference.py, other teams can easily leverage the config file to reconstruct necessary components and implement their specific inference logic.
    (4) Data sharing: the python program can leverage the ConfigParser to get constructed instance or raw config items from config file then lazy instantiation, configs can refer / extend other items in a file or even other files.

  2. Structure of the package
    (1) commands: start point to execute, like: export.sh to export the network weights, meta, config, etc. to TorchScript model, inference.sh to load TorchScript model, construct instances and execute inference. We should enhance it to something like monairun to easily support all platforms and environments.
    (2) configs: structure config for shareable components or information, can be JSON or YAML, like: metadata.json record the necessary meta information of the model package, inference.json define the args and components for inference usage.
    (3) docs: necessary documents and images, etc. like: README.md, lisence.txt, tensorboard.png, etc. depends on the model context.
    (4) models: pretrained model weights and exported TorchScript model file.
    (5) [Optional] scripts: if the model package has customized python program, like inference.py leverage the structured components to construct a special inference program.

Status

Work in progress

Checks

  • Notebook runs automatically ./runner [-p <regex_pattern>]

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma force-pushed the 486-add-model-package branch from 24c93db to fefa39a Compare December 16, 2021 02:43
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 16, 2021

I didn't implement any config-parsing related logic in it so far, marked as TODO: in the code.

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 17, 2021

Interesting comments from @ericspod during the online meeting:

  1. Just a note that .ts is meant to be Torchscript but to web developers it'll looks like a Typescript file, we may want to choose something else if anyone finds that confusing.
  2. Versioning the package would help, if we used git repos to store things the commit hash becomes part of the version implicitly.

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 20, 2021

Hi @ericspod @wyli ,

Maybe I misunderstood your suggestion, how can we get the commit hash if we want to save the commit hash in meta data for this commit...?

Thanks in advance.

@ericspod
Copy link
Member

Hi @ericspod @wyli ,

Maybe I misunderstood your suggestion, how can we get the commit hash if we want to save the commit hash in meta data for this commit...?

Thanks in advance.

What I was thinking was, from the perspective of a user, the commit hash could be used to determine model versions. It wouldn't be included in the package but would be used to tell model versions apart that claim to be the same version, ie. if the user forgets to increment the version.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Dec 21, 2021

Hi @ericspod @wyli ,
Maybe I misunderstood your suggestion, how can we get the commit hash if we want to save the commit hash in meta data for this commit...?
Thanks in advance.

What I was thinking was, from the perspective of a user, the commit hash could be used to determine model versions. It wouldn't be included in the package but would be used to tell model versions apart that claim to be the same version, ie. if the user forgets to increment the version.

Thanks for your explanation, makes sense to me!

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 17, 2022

Slightly adjusted the folder structurer, thanks for the reference of MONAI application:
https://github.com/Project-MONAI/monai-deploy/blob/main/guidelines/monai-application-package.md#table-of-important-paths

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 17, 2022

Hi @wyli @ericspod ,

Thanks for several internal discussions, I updated the example model package to show new ideas:

  1. inference.py shows how to get config and modify it then instantiation, and also shows how to get instance directly, these are the 2 ways for JSON / python hybrid.
  2. inference_v2.py shows how to write a new config with a base config and override some configs.
  3. trtinfer.json and trtinfer.py show how to refer to config in another JSON file or same file, simulated TensorRT and DALI inference logic.

Could you please help take a look again?

Thanks in advance.

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma force-pushed the 486-add-model-package branch from 18801c6 to 1b01dad Compare January 17, 2022 10:16
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 18, 2022

Hi @ericspod @wyli ,

I added the tensorboard.png and mlflow.png into the model package for training curves.

Thanks.

Signed-off-by: Nic Ma <nma@nvidia.com>
Copy link

@aihsani aihsani left a comment

Choose a reason for hiding this comment

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

Not sure why we need shell scripts in the model package... they're not easily portable across operating systems, and not descriptive in what they each achieve. Wouldn't in make more sense to remove any shell scripts and implement functionality in MONAI that is able to interpret/generate the JSON artifacts via a CLI based on the framework being used?

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 21, 2022

Hi @aihsani ,

Thanks for your suggestion!
I think the shell scripts are used to simplify the common use cases and args, especially for researchers to tune args when training a MMAR, for example, they may have several different configs, change config file in shell.
For example, the inference.sh in this example is:

python ../scripts/inference.py
    --config ../configs/inference.json
    --meta ../configs/metadata.json

And you are right, we should definitely not put complicated logic in the shell script, main things should in the python or JSON files, you can call the python program inference.py directly.

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 25, 2022

I added schema example to check the metadata format and expected values.
It's based on the jsonschema standard: http://json-schema.org/.
I didn't define schema for all the items, just included key required items as example.
We can define MONAI MMAR schema and put them into some public web storage in the future.

Thanks.

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 25, 2022

Hi @wyli ,

I changed the override feature to a separate MMAR example according to our online discussion to make it more clear.

Thanks.

Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 27, 2022

Hi @ericspod @wyli @rijobro @aihsani ,

I have updated this MMAR example according to our online discussion yesterday.
And moved the python scripts and schema to MONAI core to support 90% cases:
https://github.com/Project-MONAI/MONAI/tree/506b9089637d7dd9ed3496dde9deee8c669f269d/monai/apps/mmars

Thanks.

@ericspod
Copy link
Member

To recap what we had discussed about actual file structure, let's say we have a directory with at least these files:

models/model.ts
models/model.pt
configs/metadata.json

We can consider this directory the model package itself. The metadata.json would have in it the information that is currently provided in this PR, plus it should also have the arguments to instantiate the network rather than that being in the inference config. If the model isn't compatible with Torchscript then the mode.ts file will be absent and this will be needed to recreate the network before loading the stored weights.

This directory can then be packaged into a zip file whose structure can be assumed with models and configs directories. If the model is compatible with Torchscript then a new .ts file can be made with the metadata stored internally and other files saved as extra files using save_net_with_metadata. Users can then load this Torchscript model without worrying about the metadata or anything else MONAI specific if they just wanted to get things started quickly. Either way we'd have a zip file that's easy to distribute and which other software can expect to have as input.

@wyli
Copy link
Contributor

wyli commented Apr 27, 2022

@wyli wyli closed this Apr 27, 2022
@wyli wyli deleted the 486-add-model-package branch June 14, 2022 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add example model package
4 participants