-
Notifications
You must be signed in to change notification settings - Fork 214
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 condition param to autotune; Add feature engineering module #426
Conversation
federatedscope/autotune/algos.py
Outdated
@@ -57,7 +57,7 @@ def run(self): | |||
server_class=get_server_cls(self._trial_cfg), | |||
client_class=get_client_cls(self._trial_cfg), | |||
config=self._trial_cfg.clone(), | |||
client_config=self._client_cfgs) | |||
client_configs=self._client_cfgs) |
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.
to avoid such kinds of mistakes, we need to improve the coverage of our unit test cases
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.
Added in GitHub actions.
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.
good job! conditional search space is prevalent, and we really need it.
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.
The feature engineering module looks good to me!
@@ -64,6 +64,23 @@ def extend_data_cfg(cfg): | |||
cfg.data.quadratic.min_curv = 0.02 | |||
cfg.data.quadratic.max_curv = 12.5 | |||
|
|||
# feature engineer |
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.
It should be ''feature engineering''?
@@ -67,10 +67,10 @@ def extend_fl_setting_cfg(cfg): | |||
# ---------------------------------------------------------------------- # | |||
# Vertical FL related options (for demo) | |||
# ---------------------------------------------------------------------- # | |||
cfg.vertical_dims = [5, 10] # Avoid to be removed when `use` is False |
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.
Why we still need vertical_dims
if vertical.use=False
?
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.
xgb.dim
and vertical.dims
are merged into one arg vertical_dims
.
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 what is this? I guess it is the input dim. If it is the case, why do we need it?
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 what is this? I guess it is the input dim. If it is the case, why do we need it?
@qbc2016 Could you explain it briefly for me?
self._init_data_related_var() | ||
self.trigger_train_func(**self.kwargs_for_trigger_train_func) | ||
|
||
# Bind method to instance |
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.
It looks good! @qbc2016 Maybe we can improve some modules in FL-Tree like this
@@ -0,0 +1,14 @@ | |||
def run_scheduler(scheduler, cfg, client_cfgs=None): |
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.
please provide docstring
@@ -16,21 +16,26 @@ def run_smac(cfg, scheduler, client_cfgs=None): | |||
|
|||
def optimization_function_wrapper(config): |
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.
please supplement docstring for this method. It seems that understanding what its input is largely help figuring out what this method does.
|
||
|
||
def log2wandb(trial, config, results, trial_cfg): | ||
import wandb |
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.
As I have no idea about the overhead of repeatedly importing a module, I am wondering is it appropriate to do this? Have you benchmarked such a behavior? Here is a related thread: https://stackoverflow.com/questions/128478/should-import-statements-always-be-at-the-top-of-a-module
|
||
def wrap_instance_norm_server(worker): | ||
""" | ||
This function is to perform instance norm vfl tabular data for server. |
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.
what do you mean by "perform instance norm vfl tabular data"?
@@ -160,7 +164,7 @@ def _setup_server(self, resource_info=None, client_resource_info=None): | |||
wrap_nbafl_server | |||
wrap_nbafl_server(server) | |||
logger.info('Server has been set up ... ') | |||
return server | |||
return self.feat_engr_wrapper_server(server) |
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.
It is ok to inject those feature engineering procedures into an FL course in this way, but we shall change the instantiation of workers to a better (more general and unified) way. Actually, it is quite confusing to wrap a worker by feature engineering-related wrapper. Feature engineering is just a tiny step in an FL course, which doesn't change a worker significantly. One usual way to instantiate a worker from a collection of such pluggable behaviors is to use factory pattern I guess.
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.
approved.
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.
LGTM
Main changes
condition param
The style are shown below:
wandb
How to enable:
Install wandb and setup init configs for wandb
Run with
wandb.use True
:Feature engineering
I implement Feature engineering as a wrapper for workers.
How to use: