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

Update python_bundle_workflow #1656

Merged
merged 11 commits into from
Mar 6, 2024
Merged

Update python_bundle_workflow #1656

merged 11 commits into from
Mar 6, 2024

Conversation

KumoLiu
Copy link
Contributor

@KumoLiu KumoLiu commented Feb 28, 2024

Fixes # .

Description

  • add description for self._set_prop in python python_bundle_workflow.
  • remove generate data outside of the Workflow class.

Checks

  • Avoid including large-size files in the PR.
  • Clean up long text outputs from code cells in the notebook.
  • For security purposes, please check the contents and remove any sensitive info such as user names and private key.
  • Ensure (1) hyperlinks and markdown anchors are working (2) use relative paths for tutorial repo files (3) put figure and graphs in the ./figure folder
  • Notebook runs automatically ./runner.sh -t <path to .ipynb file>

Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
@KumoLiu KumoLiu requested review from Nic-Ma and ericspod February 28, 2024 11:05
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.

I guess this is fine to change unless there's a use for _set_props I don't see. @Nic-Ma any thoughts? Thanks!

Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

Hi @KumoLiu @ericspod ,

I remember the idea of _set_props is to save the instantiated python objects, so if some python object is not changed and you can initialize() again for some changed objects, it can reuse it.
What do you think?

Thanks.

@ericspod
Copy link
Member

Hi @KumoLiu @ericspod ,

I remember the idea of _set_props is to save the instantiated python objects, so if some python object is not changed and you can initialize() again for some changed objects, it can reuse it. What do you think?

Thanks.

Hi @Nic-Ma is this being used somewhere or will be? If it is we should write a bit more commentary since the semantics wasn't so clear. If not and won't ever be used I would take it out. Thanks!

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Feb 29, 2024

Hi @KumoLiu @ericspod ,
I remember the idea of _set_props is to save the instantiated python objects, so if some python object is not changed and you can initialize() again for some changed objects, it can reuse it. What do you think?
Thanks.

Hi @Nic-Ma is this being used somewhere or will be? If it is we should write a bit more commentary since the semantics wasn't so clear. If not and won't ever be used I would take it out. Thanks!

Hi @ericspod ,

There is a typical use-case: if we want to set the bundle idle on GPU and run inference for every input image, we can only override the data part and re-initialize the workflow, it can help avoid recreating the model and loading weights file to GPU.
@KumoLiu Can we add some comments in the example to explain?
What do you think?

Thanks.

KumoLiu and others added 7 commits March 1, 2024 10:44
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
…hub.com>

I, YunLiu <55491388+KumoLiu@users.noreply.github.com>, hereby add my Signed-off-by to this commit: cf74488
I, YunLiu <55491388+KumoLiu@users.noreply.github.com>, hereby add my Signed-off-by to this commit: 84e7629

Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
@KumoLiu
Copy link
Contributor Author

KumoLiu commented Mar 1, 2024

Hi @ericspod,
Following an offline discussion with Nic, I've updated the tutorial. The self._set_props is utilized to store user-reset initialized objects, avoiding unnecessary re-initializations.

To aid understanding and also added as a note, I've provided a brief example.

wf = TrainWorkflow()
wf.initialize()
wf.run()
wf.loss = DiceFocalLoss(sigmoid=True)
# The workflow requires re-initialization, but the user-reset loss should remain as is.
wf.initialize()

@KumoLiu KumoLiu requested review from ericspod and Nic-Ma March 1, 2024 03:31
KumoLiu added 2 commits March 4, 2024 14:51
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
@KumoLiu KumoLiu requested a review from ericspod March 4, 2024 07:07
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.

I have made a comment about the swapped order of the if-conditions in both classes, but the other changes look good to me so I can approve now and leave it up to you to swap the order of conditions or not. Thanks!

@KumoLiu KumoLiu merged commit 570b19e into Project-MONAI:main Mar 6, 2024
6 checks passed
@KumoLiu KumoLiu deleted the bundle branch March 6, 2024 01:58
boneseva pushed a commit to boneseva/MONAI-tutorials that referenced this pull request Apr 21, 2024
Fixes # .

### Description

- add description for `self._set_prop` in python
`python_bundle_workflow`.
- remove generate data outside of the Workflow class.

### Checks
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [ ] Avoid including large-size files in the PR.
- [ ] Clean up long text outputs from code cells in the notebook.
- [ ] For security purposes, please check the contents and remove any
sensitive info such as user names and private key.
- [ ] Ensure (1) hyperlinks and markdown anchors are working (2) use
relative paths for tutorial repo files (3) put figure and graphs in the
`./figure` folder
- [ ] Notebook runs automatically `./runner.sh -t <path to .ipynb file>`

---------

Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

3 participants