Make the Launcher extends FLComponent #2875
Conversation
| @@ -28,7 +29,7 @@ class LauncherRunStatus: | |||
| NOT_RUNNING = "not_running" | |||
|
|
|||
|
|
|||
| class Launcher(ABC): | |||
| class Launcher(FLComponent, ABC): | |||
There was a problem hiding this comment.
What is the motivation of this change?
Previously I assume users might be writing their own launcher without knowing NVFlare specific concepts.
But maybe that assumption is not correct.
There was a problem hiding this comment.
second that, whats the motivation of change
There was a problem hiding this comment.
This will enable the Launcher implementation class be able to handle the NVFlare events. Also be able to create the instance by "name".
There was a problem hiding this comment.
seems we are changing the component to accommodate the scan utility, and to keep the "name" feature. why not just change the name to path. are we do too much to accommodate the utility tool ? I rather not to do the change
There was a problem hiding this comment.
We could. On the other hand, I think the SubprocessLauncher should have been a FLComponent any way so that it will benefit from things like event handler.
Anyway, this is no big deal. I think we can call it done after this. Any other classes missing from the table will be added via "path".
There was a problem hiding this comment.
About YT's comment, I don't think making Launcher a FLComponent conflicts with the idea that "users might be writing their own launcher without knowing NVFlare specific concepts". User still will be able to write whatever launcher they want, just now that their launcher can now also benefit from NVFLARE's event handling mechanism.
There was a problem hiding this comment.
Besides, YT's statement seems to indicate that knowing NVFLARE specific concepts is a bad thing. I have a serious problem with this!
Knowing and understanding NVFLARE specific concepts is a wonderful thing, as long as we do not force the user to learn them before doing anything.
There was a problem hiding this comment.
Sounds good, I agree that as long as we do not force them, then it is good.
Apologize that my phrase was not accurate.
What I mean is we don't want users to be overwhelmed with our complicated (yet great) systems specifics to begin with any customization they might want to do.
This change LGTM.
In that argument, we could make some other components FLComponent as well in later release.
| @@ -28,7 +29,7 @@ class LauncherRunStatus: | |||
| NOT_RUNNING = "not_running" | |||
|
|
|||
|
|
|||
| class Launcher(ABC): | |||
| class Launcher(FLComponent, ABC): | |||
There was a problem hiding this comment.
About YT's comment, I don't think making Launcher a FLComponent conflicts with the idea that "users might be writing their own launcher without knowing NVFlare specific concepts". User still will be able to write whatever launcher they want, just now that their launcher can now also benefit from NVFLARE's event handling mechanism.
| @@ -28,7 +29,7 @@ class LauncherRunStatus: | |||
| NOT_RUNNING = "not_running" | |||
|
|
|||
|
|
|||
| class Launcher(ABC): | |||
| class Launcher(FLComponent, ABC): | |||
There was a problem hiding this comment.
Besides, YT's statement seems to indicate that knowing NVFLARE specific concepts is a bad thing. I have a serious problem with this!
Knowing and understanding NVFLARE specific concepts is a wonderful thing, as long as we do not force the user to learn them before doing anything.
|
/build |
Fixes # .
Description
Make the Launcher extends FLComponent. Print out the class loading error for module scan.
Types of changes
./runtest.sh.