change opt treatment and device policy to enum values #374
Conversation
openfl/federated/task/task_runner.py
Outdated
| @@ -103,7 +105,7 @@ def collaborator_adapted_task(col_name, round_num, input_tensor_dict, **kwargs): | |||
| validation_flag = True if task_contract['optimizer'] is None else False | |||
| task_settings = self.task_provider.task_settings[task_name] | |||
|
|
|||
| device = kwargs.get('device', 'cpu') | |||
| device = kwargs.get('device', DevicePolicy.CPU_ONLY) | |||
There was a problem hiding this comment.
This was not a device policy but a device identifier
openfl/federated/task/task_runner.py
Outdated
| @@ -214,18 +216,18 @@ def set_optimizer_treatment(self, opt_treatment): | |||
| """Change the treatment of current instance optimizer.""" | |||
| self.opt_treatment = opt_treatment | |||
|
|
|||
| def rebuild_model(self, input_tensor_dict, validation=False, device='cpu'): | |||
| def rebuild_model(self, input_tensor_dict, validation=False, device=DevicePolicy.CPU_ONLY): | |||
There was a problem hiding this comment.
This also should be reverted
openfl/federated/task/task_runner.py
Outdated
| @@ -358,7 +360,7 @@ def get_tensor_dict(self, with_opt_vars=False): | |||
|
|
|||
| return self.framework_adapter.get_tensor_dict(*args) | |||
|
|
|||
| def set_tensor_dict(self, tensor_dict, with_opt_vars=False, device='cpu'): | |||
| def set_tensor_dict(self, tensor_dict, with_opt_vars=False, device=DevicePolicy.CPU_ONLY): | |||
There was a problem hiding this comment.
not a policy but a device
| @@ -104,7 +106,8 @@ def _rebuild_model(self, tensor_dict, upcoming_model_status=ModelStatus.BEST): | |||
| self.logger.warning(warning_msg) | |||
|
|
|||
| else: | |||
| self.task_runner_stub.rebuild_model(tensor_dict, validation=True, device='cpu') | |||
| self.task_runner_stub.rebuild_model( | |||
| tensor_dict, validation=True, device=DevicePolicy.CPU_ONLY) | |||
| delta_updates=delta_updates, opt_treatment=opt_treatment.name, | ||
| device_assignment_policy=device_assignment_policy.name, |
There was a problem hiding this comment.
We may convert those later, before saving to plan or sending through gRPC
| delta_updates=delta_updates, opt_treatment=opt_treatment.name, | |
| device_assignment_policy=device_assignment_policy.name, | |
| delta_updates=delta_updates, opt_treatment=opt_treatment, | |
| device_assignment_policy=device_assignment_policy, |
|
Jenkins please retry a build |
| if isinstance(opt_treatment, str) and hasattr(OptTreatment, opt_treatment): | ||
| defaults[SETTINGS]['opt_treatment'] = OptTreatment[opt_treatment].value | ||
| elif isinstance(opt_treatment, int) and OptTreatment(opt_treatment): | ||
| pass |
There was a problem hiding this comment.
Shouldn't we parse integer value here?
There was a problem hiding this comment.
if opt_treatment is already an integer (from lime 463), then it's already parsed
There was a problem hiding this comment.
Why do we need this check then?
There was a problem hiding this comment.
So both integer and string values are appropriate here if there are multiple collaborators. In both of these conditions, it shouldn't raise an exception but for all other cases it should.
| @@ -166,8 +170,8 @@ def start(self, *, model_provider, task_keeper, data_loader, | |||
| rounds_to_train: int, | |||
| task_assigner=None, | |||
| delta_updates: bool = False, | |||
| opt_treatment: str = 'RESET', | |||
| device_assignment_policy: str = 'CPU_ONLY', | |||
| opt_treatment: Enum = OptTreatment.RESET, | |||
There was a problem hiding this comment.
| opt_treatment: Enum = OptTreatment.RESET, | |
| opt_treatment: OptTreatment = OptTreatment.RESET, |
| opt_treatment: str = 'RESET', | ||
| device_assignment_policy: str = 'CPU_ONLY', | ||
| opt_treatment: Enum = OptTreatment.RESET, | ||
| device_assignment_policy: Enum = DevicePolicy.CPU_ONLY, |
There was a problem hiding this comment.
| device_assignment_policy: Enum = DevicePolicy.CPU_ONLY, | |
| device_assignment_policy: DevicePolicy = DevicePolicy.CPU_ONLY, |
| @@ -368,7 +369,7 @@ | |||
| "name": "python", | |||
| "nbconvert_exporter": "python", | |||
| "pygments_lexer": "ipython3", | |||
| "version": "3.8.8" | |||
| "version": "3.8.9" | |||
There was a problem hiding this comment.
Please clear the notebook metadata
| @@ -484,12 +484,14 @@ | |||
| "# If I use autoreload I got a pickling error\n", | |||
| "\n", | |||
| "# The following command zips the workspace and python requirements to be transfered to collaborator nodes\n", | |||
| "from openfl.utilities.enum_types import DevicePolicy, OptTreatment\n", | |||
There was a problem hiding this comment.
Please split this import into separate lines
|
|
||
| opt_treatment = defaults[SETTINGS]['opt_treatment'] | ||
| if isinstance(opt_treatment, str) and hasattr(OptTreatment, opt_treatment): | ||
| defaults[SETTINGS]['opt_treatment'] = OptTreatment[opt_treatment].value |
There was a problem hiding this comment.
Maybe better to save string here to easeir review plan.yaml.
In experiment it is using plan.config['collaborator']['settings']['opt_treatment'] = opt_treatment.name.
We should save values in plan in the same format.
There was a problem hiding this comment.
Yes I and @igor-davidyuk discussed about it and thought of using value instead of name
|
Jenkins please retry a build |
|
@mansishr can you please resolve the conflicts and request for review again? |
Fixes #194