-
Notifications
You must be signed in to change notification settings - Fork 181
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
base: main
Are you sure you want to change the base?
Add BaseFedJob layer #3098
Conversation
@@ -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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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?
91a0bea
to
2a8826f
Compare
/build |
Add framework agnostic BaseFedJob layer, rename current classes to PTBaseFedJob and TFBaseFedJob
Types of changes
./runtest.sh
.