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 BaseFedJob layer #3098

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add BaseFedJob layer #3098

wants to merge 2 commits into from

Conversation

SYangster
Copy link
Collaborator

@SYangster SYangster commented Dec 10, 2024

Add framework agnostic BaseFedJob layer, rename current classes to PTBaseFedJob and TFBaseFedJob

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.

@@ -335,10 +335,10 @@
"from src.lit_net import LitNet\n",
"\n",
"from nvflare.app_common.workflows.fedavg import FedAvg\n",
"from nvflare.app_opt.pt.job_config.base_fed_job import BaseFedJob\n",
"from nvflare.app_opt.pt.job_config.base_fed_job import PTBaseFedJob\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, are there PTFedJob and TFFedJobs as well? Maybe we can skip the "Base" in the name in these training framework specific job classes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We currently don't have PTFedJob/TFFedJobs. We could remove the "Base" in these names, would this class naming still make sense?

FedJob -> BaseFedJob -> PTFedJob

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another proposal: Does this: PTJob -> CommonJob -> FedJob make sense?

Maybe we don’t need to make the names suggest the parent class they inherit from.

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.

Logic is good, naming can discuss with team, from the name would BaseFedJob suggest it is more basic than FedJob?

@SYangster
Copy link
Collaborator Author

/build

@SYangster SYangster marked this pull request as draft December 16, 2024 19:28
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