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

Enable run cross-validation without training workflow and examples #2035

Merged
merged 30 commits into from
Dec 7, 2023
Merged

Enable run cross-validation without training workflow and examples #2035

merged 30 commits into from
Dec 7, 2023

Conversation

yhwen
Copy link
Collaborator

@yhwen yhwen commented Sep 26, 2023

…xample for demonstration.

Fixes # .

Description

To enable re-run cross-site validation without the training workflow. Also include an cifar10_fedavg example to demonstrate how to configure and run cross-site validation indepentelty.

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.

chesterxgchen
chesterxgchen requested changes Sep 26, 2023
View reviewed changes
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.

the example seems duplicate the CIFAR10 examples with a lot details that is not needed.

Can we simplify this

@yhwen
Copy link
Collaborator Author

yhwen commented Sep 27, 2023

the example seems duplicate the CIFAR10 examples with a lot details that is not needed.

Can we simplify this

the example seems duplicate the CIFAR10 examples with a lot details that is not needed.

Can we simplify this

I will explain in the meeting what's the point to make the cross-site validation re-run without training workflow work. And how is this example built. (exact the same as the cifar10_fedavg)

@YuanTingHsieh YuanTingHsieh linked an issue Nov 8, 2023 that may be closed by this pull request
Cross-site validation / model validation enhancement #1905
Closed
yanchengnv
yanchengnv requested changes Nov 29, 2023
View reviewed changes
Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

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

I unresolved some issues raised before.

nvflare/app_opt/pt/file_list_model_locator.py Outdated Show resolved Hide resolved
nvflare/app_common/np/np_model_locator.py Outdated Show resolved Hide resolved
nvflare/app_opt/pt/file_list_model_locator.py Outdated Show resolved Hide resolved
@yhwen
Copy link
Collaborator Author

yhwen commented Nov 29, 2023

I unresolved some issues raised before.

@yhwen yhwen closed this Nov 29, 2023
@yhwen yhwen reopened this Nov 29, 2023
yanchengnv
yanchengnv requested changes Nov 29, 2023
View reviewed changes
Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

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

Some additional changes needed in np_model_locator. See comments.

nvflare/app_common/np/np_model_locator.py Outdated Show resolved Hide resolved
nvflare/app_common/np/np_model_locator.py Outdated Show resolved Hide resolved
yanchengnv
yanchengnv previously approved these changes Nov 30, 2023
View reviewed changes
Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

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

LGTM

YuanTingHsieh
YuanTingHsieh reviewed Dec 4, 2023
View reviewed changes
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.

Overall LGTM.
My only concern is those binary files.

Could it be possible we have some scripts to generate this?

For example:

mock_train.py

import numpy as np

result = np.array(
  [1,2,3]
)

np.save(result)

This way it might be clearer for the users.
We can do this in another PR as well.

YuanTingHsieh
YuanTingHsieh previously approved these changes Dec 5, 2023
View reviewed changes
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.

We can address the data preparation in next PR.

@yhwen
Copy link
Collaborator Author

yhwen commented Dec 6, 2023

I unresolved some issues raised before.

We can address the data preparation in next PR.

Changed to use a script to generate the pre-trained models. Removed the binary model files.

chesterxgchen
chesterxgchen approved these changes Dec 6, 2023
View reviewed changes
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.

LGTM with minor readme wording changes

@yhwen
Copy link
Collaborator Author

yhwen commented Dec 6, 2023

/build

@YuanTingHsieh
Copy link
Collaborator

/build

@YuanTingHsieh YuanTingHsieh merged commit 28768e1 into NVIDIA:main Dec 7, 2023
16 checks passed
@yhwen yhwen deleted the rerun_cross_validation branch May 20, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SYangster SYangster SYangster left review comments

@YuanTingHsieh YuanTingHsieh YuanTingHsieh left review comments

@holgerroth holgerroth holgerroth left review comments

@chesterxgchen chesterxgchen chesterxgchen approved these changes

@yanchengnv yanchengnv yanchengnv left review comments

@IsaacYangSLA IsaacYangSLA Awaiting requested review from IsaacYangSLA

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

Successfully merging this pull request may close these issues.

Cross-site validation / model validation enhancement
6 participants
@yhwen @yanchengnv @chesterxgchen @YuanTingHsieh @holgerroth @SYangster