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

Bundle Specification Needs Required Packages #6959

Closed
ericspod opened this issue Sep 7, 2023 · 7 comments · Fixed by #7253
Closed

Bundle Specification Needs Required Packages #6959

ericspod opened this issue Sep 7, 2023 · 7 comments · Fixed by #7253
Assignees
Labels
Bundles Anything related to bundles enhancement New feature or request

Comments

@ericspod
Copy link
Member

ericspod commented Sep 7, 2023

Is your feature request related to a problem? Please describe.
Bundles can be specified to depend on optional packages through the optional_packages_version in metadata.json. Some bundles may require certain packages so a required_packages_version may be needed, or a change in name for optional_packages_version.

Describe the solution you'd like
Some way of specifying what packages other Pytorch, Numpy, and MONAI are needed by a bundle.

Describe alternatives you've considered
I've used optional_packages_version to specify needed packages but this doesn't seem adequate.

@ericspod ericspod added enhancement New feature or request Bundles Anything related to bundles labels Sep 7, 2023
@ericspod
Copy link
Member Author

ericspod commented Sep 7, 2023

I don't think this is a really high priority item, but we should update the metadata schema some time anyway.

@KumoLiu
Copy link
Contributor

KumoLiu commented Nov 21, 2023

Hi @ericspod, for now, if optional_packages_version is in "metadata.json", it will add these packages to requirements.txt. Doesn't this currently meet the request, or do you think it would be better to change the name to required_packages_version?
https://github.com/Project-MONAI/model-zoo/blob/8845f3a04766e254b156c23fc4d2f91659c056d4/ci/get_bundle_requirements.py#L39

@ericspod
Copy link
Member Author

The issue is that there are required packages that a bundle absolutely needs and packages that would nice to have but not strictly needed. These two aren't differentiated in the json file and I felt there should be, or we change the name to required_packages_version to make it clear these packages are needed and not worry about having things optional.

@KumoLiu KumoLiu self-assigned this Nov 23, 2023
KumoLiu added a commit to KumoLiu/MONAI that referenced this issue Nov 23, 2023
Signed-off-by: KumoLiu <yunl@nvidia.com>
@KumoLiu
Copy link
Contributor

KumoLiu commented Nov 23, 2023

Hi @Ericspo, I think we also need to create a new schema and update all bundles. What do you think?
https://github.com/Project-MONAI/model-zoo/blob/f20f894f2c0993c94184c7002529ea303a2f8c74/models/brats_mri_segmentation/configs/metadata.json#L2C16-L2C120

@ericspod
Copy link
Member Author

Yes we do need a new schema. I wanted to update it to try to take into account non-tensor arguments as well as arbitrary argument names. It currently isn't quite correct without so I need to do some looking into JSON schema definitions and get to that.

@KumoLiu
Copy link
Contributor

KumoLiu commented Nov 24, 2023

What non-tensor arguments and arbitrary argument names did you refer to?
I also considering updating the output definition in the schema. For the raw output of the network, it is possible that there is more than just a single tensor, do you think we need to consider them all? Look like now we are defining the raw output of the network. But sometimes the output can be more complicated(such as detection).
#6724
https://github.com/Project-MONAI/model-zoo/blob/f20f894f2c0993c94184c7002529ea303a2f8c74/models/endoscopic_tool_segmentation/configs/metadata.json#L78

@ericspod
Copy link
Member Author

What non-tensor arguments and arbitrary argument names did you refer to?

Currently the schema allows only tensor arguments to a network's forward pass and we need to take into account other types. The initial design intent was to capture what tensor values are needed for input but to be a complete specification we need to account for other things. For outputs it might be the same thing where we need to account for more than just tensors as output, and different numbers of outputs.

ericspod added a commit that referenced this issue Jul 25, 2024
Fixes #7303 #6959.

### Description

This adds the schema file into the code base (but this maybe should be
elsewhere). The changes implement a number of new things:

* Moved definitions into a `$defs` section per the JSON schema standard
* Permits multiple input arguments and return results from networks with
arbitrary names using the `patternProperties` mechanism
* Allows the types of inputs and outputs to be, additional to just
tensors, numbers, booleans, or strings
* Outputs after post processing can be specified with the
`post_processed_outputs` section if they are significantly changed with
the post-process transforms defined in scripts
* Multiple network IO formats can be specified in addition to
`network_data_format`, these must follow the pattern
`<name>_data_format`
* `required_packages_version` added in addition to
`optional_packages_version`

#7253 depends on this schema change.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] 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.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: Eric Kerfoot <eric.kerfoot@kcl.ac.uk>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yiheng Wang <68361391+yiheng-wang-nv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bundles Anything related to bundles enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants