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

Rename optional_packages_version to required_packages_version #7253

Merged
merged 16 commits into from
Jul 30, 2024

Conversation

KumoLiu
Copy link
Contributor

@KumoLiu KumoLiu commented Nov 23, 2023

Fixes #6959.

Description

A few sentences describing the changes proposed in this pull request.

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.
  • 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: KumoLiu <yunl@nvidia.com>
@KumoLiu KumoLiu marked this pull request as draft November 23, 2023 10:32
@ericspod
Copy link
Member

Thanks! We should add something to the ConfigParser which recognises optional_packages_version as required_packages_version and issues a warning. The old name should have a deprecation version as well so perhaps we should have a formal structure for designating synonyms and deprecation, eg. some table in the code denoting these things. @Nic-Ma @wyli Thoughts?

Signed-off-by: KumoLiu <yunl@nvidia.com>
@KumoLiu
Copy link
Contributor Author

KumoLiu commented Nov 27, 2023

We should add something to the ConfigParser which recognises optional_packages_version as required_packages_version and issues a warning.

Hi @ericspod, I recognize optional_packages_version as required_packages_version and issue a warning in the latest commit. Could you please help review it again?
The CI would still fail since the schema is not updated yet.

KumoLiu and others added 3 commits November 27, 2023 16:53
Signed-off-by: KumoLiu <yunl@nvidia.com>
Signed-off-by: KumoLiu <yunl@nvidia.com>
@ericspod
Copy link
Member

Looks good! We should get some more feedback from others however since this will change things and would necessitate a model zoo update.

KumoLiu and others added 2 commits November 29, 2023 09:51
Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
@KumoLiu KumoLiu requested a review from Nic-Ma November 29, 2023 01:59
KumoLiu and others added 2 commits November 29, 2023 10:01
Signed-off-by: KumoLiu <yunl@nvidia.com>
@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jan 15, 2024

Any blocker in this MR?

Thanks.

@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jan 15, 2024

Details

Hi @Nic-Ma, yes we need to update the schema before merging this PR.
Here is the ticket.
#7303

@ericspod
Copy link
Member

@KumoLiu @Nic-Ma I have an update for the schema stated in comments here: #7303

Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
ericspod added a commit that referenced this pull request 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>
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
@KumoLiu KumoLiu marked this pull request as ready for review July 26, 2024 08:00
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Copy link
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

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

Looks good here, thanks.

@ericspod ericspod enabled auto-merge (squash) July 30, 2024 10:08
@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jul 30, 2024

/build

@ericspod ericspod merged commit f1ef3e8 into Project-MONAI:dev Jul 30, 2024
28 checks passed
@KumoLiu KumoLiu deleted the bundle-package branch July 30, 2024 13:59
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.

Bundle Specification Needs Required Packages
4 participants