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

[Auto3DSeg] Add mlflow support in autorunner. #7176

Merged
merged 10 commits into from
Nov 2, 2023

Conversation

dongyang0122
Copy link
Collaborator

@dongyang0122 dongyang0122 commented Oct 31, 2023

Add MLflow support in AutoRunner Class.

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: dongy <dongy@nvidia.com>
dongy added 2 commits October 31, 2023 11:17
Signed-off-by: dongy <dongy@nvidia.com>
@dongyang0122 dongyang0122 changed the title [auto3dseg] Add mlflow support in autorunner. [Auto3DSeg] Add mlflow support in autorunner. Oct 31, 2023
monai/apps/auto3dseg/auto_runner.py Show resolved Hide resolved
monai/apps/auto3dseg/bundle_gen.py Outdated Show resolved Hide resolved
@myron
Copy link
Collaborator

myron commented Oct 31, 2023

I think this should be a part of kwargs of Autorunner, instead of adding more names parameters for very specific use case.

Then you check for it as
If kwargs.get('mlflow_url', None) is not None:

@drbeh
Copy link
Member

drbeh commented Oct 31, 2023

I think this should be a part of kwargs of Autorunner, instead of adding more names parameters for very specific use case.

Then you check for it as If kwargs.get('mlflow_url', None) is not None:

@myron @dongyang0122 As the zen of python says "explicit is better than implicit". In my opinion, having more names means better documentation. Deferring to kwargs usually makes sense when dealing with a third party parameters that might change over time without our notice, but mlflow_tracking_uri is our parameter and we decide how to use it (although we are passing it to mlflow).

dongy and others added 4 commits October 31, 2023 16:48
Signed-off-by: dongy <dongy@nvidia.com>
Signed-off-by: dongy <dongy@nvidia.com>
Signed-off-by: dongy <dongy@nvidia.com>
@dongyang0122
Copy link
Collaborator Author

I think this should be a part of kwargs of Autorunner, instead of adding more names parameters for very specific use case.
Then you check for it as If kwargs.get('mlflow_url', None) is not None:

@myron @dongyang0122 As the zen of python says "explicit is better than implicit". In my opinion, having more names means better documentation. Deferring to kwargs usually makes sense when dealing with a third party parameters that might change over time without our notice, but mlflow_tracking_uri is our parameter and we decide how to use it (although we are passing it to mlflow).

I acknowledge the perspectives presented from both sides. Incorporating an input variable mlflow_tracking_uri would enhance parameter visibility and documentation, whereas including it under ‘kwargs’ could conserve script space. Personally, I favor the first approach, though both methods are viable. Input from individuals @Nic-Ma and @daguangxu on this matter would be beneficial to finalize the decision.

dongy added 2 commits November 1, 2023 15:39
Signed-off-by: dongy <dongy@nvidia.com>
Signed-off-by: dongy <dongy@nvidia.com>
@Nic-Ma
Copy link
Contributor

Nic-Ma commented Nov 2, 2023

Hi @drbeh , @dongyang0122 ,

Do we have any usecase to allow users to set experiment_name? If no, I would vote for current implementation with mlflow_tracking_uri arg directly.

Thanks.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Nov 2, 2023

/black

@wyli
Copy link
Contributor

wyli commented Nov 2, 2023

/build

@wyli wyli enabled auto-merge (squash) November 2, 2023 08:25
@wyli wyli disabled auto-merge November 2, 2023 08:41
@wyli
Copy link
Contributor

wyli commented Nov 2, 2023

/build

1 similar comment
@wyli
Copy link
Contributor

wyli commented Nov 2, 2023

/build

@wyli wyli enabled auto-merge (squash) November 2, 2023 08:57
@wyli wyli merged commit c3f9914 into Project-MONAI:dev Nov 2, 2023
28 checks passed
marksgraham pushed a commit to marksgraham/MONAI that referenced this pull request Jan 30, 2024
Add MLflow support in AutoRunner Class.

### 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: dongy <dongy@nvidia.com>
Signed-off-by: Mark Graham <markgraham539@gmail.com>
Yu0610 pushed a commit to Yu0610/MONAI that referenced this pull request Apr 11, 2024
Add MLflow support in AutoRunner Class.

### 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: dongy <dongy@nvidia.com>
Signed-off-by: Yu0610 <612410030@alum.ccu.edu.tw>
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.

None yet

5 participants