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.

Sign up for GitHub

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

Jump to bottom

Rename optional_packages_version to required_packages_version #7253

Merged
merged 16 commits into from
Jul 30, 2024
Merged

Rename optional_packages_version to required_packages_version #7253

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.

@KumoLiu
fix Project-MONAI#6959
4c0f382 
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?

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

KumoLiu commented Nov 27, 2023 •
edited
Loading

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 KumoLiu requested a review from ericspod November 27, 2023 08:47
KumoLiu and others added 3 commits November 27, 2023 16:53
@KumoLiu
fix format
5587ee3 
Signed-off-by: KumoLiu <yunl@nvidia.com>
@KumoLiu
fix ci
ec231c8 
Signed-off-by: KumoLiu <yunl@nvidia.com>
@KumoLiu
Merge branch 'dev' into bundle-package
c12a27f
@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
@KumoLiu @ericspod
Update monai/bundle/reference_resolver.py
d3b9071 
Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
@KumoLiu @ericspod
Update monai/bundle/reference_resolver.py
e916f8e 
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
@KumoLiu
fix ci
a9bb50b 
Signed-off-by: KumoLiu <yunl@nvidia.com>
@KumoLiu
Merge branch 'dev' into bundle-package
5f6345c
@KumoLiu KumoLiu mentioned this pull request Dec 8, 2023
Closed
@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

@ericspod ericspod mentioned this pull request Jan 22, 2024
Merged
7 tasks
KumoLiu added 2 commits July 24, 2024 17:03
@KumoLiu
Merge branch 'dev' into bundle-package
9f27668
@KumoLiu
fix format
d92b196 
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
ericspod added a commit that referenced this pull request Jul 25, 2024
@ericspod @pre-commit-ci @yiheng-wang-nv
Adding metadata schema to the code base itself (#7409)
2e53df7 
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>
KumoLiu added 3 commits July 26, 2024 15:46
@KumoLiu
Merge branch 'dev' into bundle-package
74b646a
@KumoLiu
Merge remote-tracking branch 'origin/dev' into bundle-package
c469413 
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
@KumoLiu
update schema
612984a 
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
@KumoLiu
update test data
c7551d1 
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
ericspod
ericspod approved these changes Jul 30, 2024
View reviewed changes
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
Reviewers

@ericspod ericspod ericspod approved these changes

@Nic-Ma Nic-Ma Awaiting requested review from Nic-Ma

@mingxin-zheng mingxin-zheng Awaiting requested review from mingxin-zheng

@yiheng-wang-nv yiheng-wang-nv Awaiting requested review from yiheng-wang-nv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bundle Specification Needs Required Packages
4 participants
@KumoLiu @ericspod @Nic-Ma @mingxin-zheng