Workspace context manager #148
Conversation
There was a problem hiding this comment.
Could we combine CollaboratorWorkspace and AggregatorWorkspace, maybe introduce an additional parameter.
At least make a base ActorWorkspace class with common functionality?
Also could you please remove the old version in this PR?
https://github.com/aleksandr-mokrov/openfl/blob/e04c2e00b3bd072ff23e06b4c5a36e4de768020d/openfl/utilities/utils.py#L56
| from pathlib import Path | ||
|
|
||
|
|
||
| class AggregatorWorkspace: |
There was a problem hiding this comment.
I suggest moving this away from the components package, to the utilities probably
There was a problem hiding this comment.
Moved to utilities/workspace.py
openfl/component/envoy/workspace.py
Outdated
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class CollaboratorWorkspace: |
There was a problem hiding this comment.
I suggest moving this away from the components package too.
There was a problem hiding this comment.
Moved to utilities/workspace.py
|
| self, | ||
| experiment_name: str, | ||
| data_file_path: Path, | ||
| is_install_requirements: bool = False |
There was a problem hiding this comment.
is_install_requirements -> install_requirements
probably this will look better
There was a problem hiding this comment.
It's boolean flag. It will be strange:
if self.install_requirements:
self._install_requirements()Probably is_install_requirements_required would be better, but too long.
There was a problem hiding this comment.
I also think that requirements will be better. You still can rename it inside __init__.
| with ExperimentWorkspace( | ||
| experiment_name, data_file_path, is_install_requirements=True | ||
| ): |
There was a problem hiding this comment.
Maybe rename is_install_requirements => requirements in order to avoid so long line.
| def wait_experiment(self): | ||
| """Wait an experiment data from the director.""" |
There was a problem hiding this comment.
Maybe wait_experiment should be get_experiment_name?
| self, | ||
| experiment_name: str, | ||
| data_file_path: Path, | ||
| is_install_requirements: bool = False |
There was a problem hiding this comment.
I also think that requirements will be better. You still can rename it inside __init__.
|
|
||
| def _install_requirements(self): | ||
| """Install experiment requirements.""" | ||
| requirements_filename = f'{self.experiment_work_dir}/requirements.txt' |
There was a problem hiding this comment.
Maybe better to use pathlib.Path here
There was a problem hiding this comment.
This variable is used in 3 places in this method, some of them are converted to a string, and some must be converted to a string manually. None of them use type Path.
|
|
||
| if os.path.isfile(requirements_filename): | ||
| attempts = 10 | ||
| for _ in range(attempts): |
There was a problem hiding this comment.
@aleksandr-mokrov Shouldn't we raise an exception when all attempts to install the requirements failed?
Use context managers to manage experiment workspaces
Fix plan.config['task_runner'] filling.