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

Add missing docs and pieces for experiment tracking [skip ci] #1981

Merged
merged 20 commits into from
Sep 19, 2023

Conversation

nvkevlu
Copy link
Collaborator

@nvkevlu nvkevlu commented Sep 8, 2023

Add missing docs and pieces for experiment tracking. Adds examples using MetricsExchanger.

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.
  • Quick tests passed locally by running ./runtest.sh.
  • In-line docstrings updated.
  • Documentation updated.

@nvkevlu nvkevlu marked this pull request as draft September 8, 2023 17:00
@nvkevlu nvkevlu marked this pull request as ready for review September 18, 2023 16:27
@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Sep 18, 2023

/build

docs/user_guide/dashboard_ui.rst Outdated Show resolved Hide resolved
docs/user_guide/dashboard_ui.rst Outdated Show resolved Hide resolved
docs/user_guide/nvflare_security.rst Outdated Show resolved Hide resolved
examples/advanced/experiment-tracking/README.md Outdated Show resolved Hide resolved
examples/advanced/experiment-tracking/README.md Outdated Show resolved Hide resolved
nvflare/app_common/tracking/log_writer_me.py Show resolved Hide resolved
Copy link
Collaborator

@chesterxgchen chesterxgchen left a comment

Choose a reason for hiding this comment

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

added few comments

@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Sep 19, 2023

/build

@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Sep 19, 2023

/build

Copy link
Collaborator

@chesterxgchen chesterxgchen left a comment

Choose a reason for hiding this comment

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

Overall looks good.

The only comment is related to the MericsExchanger/MetricsReciever. These are internal implementation details. They shouldn't need to expose to the user. The implementation could be changed later.

User just want to log metrics to stream to MLflow/WandB etc. their experience should be the same regardless they in learner class or train.py class. The only difference should be import package

For Monai, the important point, is that, if user uses FLARE + MONAI + MLFLow, the metrics defined in Monai bundle will be automatically sent over to MLFLOW via FLARE Server.

@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Sep 19, 2023

/build

@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Sep 19, 2023

/build

@nvkevlu nvkevlu changed the title Add missing docs and pieces for experiment tracking Add missing docs and pieces for experiment tracking [skip ci] Sep 19, 2023
@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Sep 19, 2023

/build

@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Sep 19, 2023

/build

Copy link
Collaborator

@YuanTingHsieh YuanTingHsieh left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@nvkevlu nvkevlu merged commit 23f2245 into NVIDIA:main Sep 19, 2023
14 checks passed
@nvkevlu nvkevlu deleted the add_exp_trk branch September 19, 2023 22:44
holgerroth pushed a commit to holgerroth/NVFlare that referenced this pull request Dec 4, 2023
…#1981)

* add missing docs and pieces for experiment tracking

* fix ci and update monai readme

* fix configs

* rename log to write_log to avoid conflict with wandb log

* update mlflow config

* make changes for comments, add missing changes

* fix ci

* remove submit_job scripts to use the cli command instead

* address more PR comments

* revise wording based on PR comment

* revise wording based on PR comment

* remove metrics-exchanger from example
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