[Workflow Interface] Rename and Refactor WorkspaceExport into NotebookTools #1364
Conversation
Signed-off-by: refai06 <[email protected]>
Signed-off-by: refai06 <[email protected]>
refai06
requested review from
MasterSkepticista,
kta-intel,
psfoley,
teoparvanov and
rahulga1
as code owners
There was a problem hiding this comment.
Could having an overloaded NotebookTools.export function contribute to a more streamlined user interface? With the introduction of SecureFederatedRuntime (and other runtime environments in the future), users could leverage a single export call (with varying parameters), allowing the system to internally determine the appropriate export flow for each runtime.
There was a problem hiding this comment.
@teoparvanov @scngupta-dsp What are your thoughts on this?
There was a problem hiding this comment.
Good point, @theakshaypant - indeed, the current interface would not scale very well with the addition of more runtimes...
Signed-off-by: refai06 <[email protected]>
Signed-off-by: refai06 <[email protected]>
Signed-off-by: refai06 <[email protected]>
Signed-off-by: refai06 <[email protected]>
refai06
requested review from
noopurintel,
payalcha,
rajithkrishnegowda,
ishaileshpant and
tanwarsh
as code owners
refai06
changed the title
openfl-tutorials/experimental/workflow/1001_Workspace_Creation_from_JupyterNotebook.ipynb
Outdated
Show resolved
Hide resolved
Signed-off-by: refai06 <[email protected]>
refai06
requested review from
aayushgaintel,
gbikkiintel,
pasokan-intel and
srikanthenugul
as code owners
...l/experimental/workflow/NotebookTools/testcase_export/test_artifacts/expected/plan/cols.yaml
Outdated
Show resolved
Hide resolved
.../experimental/workflow/NotebookTools/testcase_export/test_artifacts/expected/src/__init__.py
Outdated
Show resolved
Hide resolved
Signed-off-by: refai06 <[email protected]>
Signed-off-by: refai06 <[email protected]>
| @@ -0,0 +1,5 @@ | |||
| # Copyright (C) 2020-2021 Intel Corporation | |||
| # Licensed subject to the terms of the separately executed evaluation license agreement between Intel Corporation and you. | |||
There was a problem hiding this comment.
As discussed - Instead of using expected. Can we have proper asserts or conditions in the code. So that hardcoded files just to compare is not needed.
| import pytest | ||
| from openfl.experimental.workflow.notebooktools import NotebookTools | ||
|
|
||
| class bcolors: |
There was a problem hiding this comment.
Please use testing framework like unittest or pytest to run.
There was a problem hiding this comment.
Good point, @theakshaypant - indeed, the current interface would not scale very well with the addition of more runtimes...
| @classmethod | ||
| def export_federated( | ||
| cls, notebook_path: str, output_workspace: str, director_fqdn: str, tls: bool = False | ||
| ) -> Tuple[str, str]: | ||
| """Exports workspace for FederatedRuntime. | ||
|
|
||
| Args: | ||
| notebook_path (str): Path to the Jupyter notebook. | ||
| output_workspace (str): Path for the generated workspace directory. | ||
| director_fqdn (str): Fully qualified domain name of the director node. | ||
| tls (bool, optional): Whether to use TLS for the connection. | ||
|
|
||
| Returns: | ||
| Tuple[str, str]: A tuple containing: | ||
| (archive_path, flow_class_name). | ||
| """ | ||
| instance = cls(notebook_path, output_workspace) | ||
| instance._generate_requirements() | ||
| instance._generate_plan_yaml(director_fqdn, tls) | ||
| instance._clean_generated_workspace() | ||
| print_tree(output_workspace, level=2) | ||
| return instance._generate_experiment_archive() | ||
|
|
||
| @classmethod | ||
| def export(cls, notebook_path: str, output_workspace: str) -> None: | ||
| """Exports workspace to output_workspace. | ||
| Args: | ||
| notebook_path (str): Path to the Jupyter notebook. | ||
| output_workspace (str): Path for the generated workspace directory. | ||
| """ | ||
| instance = cls(notebook_path, output_workspace) | ||
| instance._generate_requirements() | ||
| instance._generate_plan_yaml() | ||
| instance._generate_data_yaml() | ||
| print_tree(output_workspace, level=2) |
There was a problem hiding this comment.
I have to admit that I don't understand very well what's going on here... You have a NotebookTools class that maintains some internal state (self.notebook_path, self.output_workspace_path, etc.). At the same time the public methods of this class are "static" (@classmethod).
Going into more detail, I also have trouble grasping the semantic difference between export() and export_federated(). Is it so significant that it warrants a separate method for the two? If you need different behaviors between LocalRuntime and FederatedRuntime, you could maybe consider creating sub-classes of NotebookTools for those two cases? As pointed out by @theakshaypant, this would also enable accommodating SecureFederatedRuntime down the road. However, I'm not sure if the differences are indeed that significant to warrant the added complexity in the class hierarchy...
WDYT?
| logger = getLogger(__name__) | ||
|
|
||
|
|
||
| class NotebookTools: |
There was a problem hiding this comment.
The name NotebookTools implies an amorphous collection of tools/methods, rather than a well-encapsulated entity. I suggest renaming this class to something like FLWorkflowNotebook, with a single export_to(workspace_output_path: str) public method.
Here is an illustration of the proposed API and its usage:
nb = FLWorkflowNotebook("/home/dev/experiment.ipynb")
nb.export_to("/home/dev/fl_workspace")Signed-off-by: refai06 <[email protected]>
Description:
This PR refactors
WorkspaceExportfunctionality into distinct classes:NotebookToolsandCodeAnalyzer. This change is intended to improve code organization, maintainability, and readability to user.Change Description:
NotebookTools:The main interface for workspace export and management operations.
Handles two export interfaces:
export(): Aggregator-based workflow export.export_federated(): FederatedRuntime-specific export.Responsible for generating configuration and requirements files.
CodeAnalyzer:A dedicated helper class for code analysis and transformation.
Provides utilities for converting notebooks to Python scripts, analysing and extracting key information, including flow details, configurations, runtime details.
Note:
NotebookToolsprovides the user interface (for Aggregator based workflow) and internally utilizes theCodeAnalyzerclass.NotebookToolsreplaces theWorkspaceExportmodule.Deleted:
openfl/experimental/workflow/workspace_export/export.pyFiles Added:
openfl/experimental/workflow/notebooktools/notebook_tools.pyopenfl/experimental/workflow/notebooktools/code_analyzer.pyFiles Modified:
openfl-tutorials/experimental/workflow/1001_Workspace_Creation_from_JupyterNotebook.ipynbopenfl-tutorials/experimental/workflow/Vertical_FL/TwoPartyWorkspaceCreation.ipynbopenfl/experimental/workflow/interface/cli/workspace.pyopenfl/experimental/workflow/runtime/federated_runtime.pyVerification:
Following tutorials are validated to ensure the refactoring is working fine
openfl-tutorials/experimental/workflow/FederatedRuntime/301_MNIST_Watermakingopenfl-tutorials/experimental/workflow/1001_Workspace_Creation_from_JupyterNotebook.ipynbTest Case:
Following test cases are added which validates the NotebookTools
export()andexport_federated()functionalitytests/openfl/experimental/workflow/NotebookTools/tescase_exporttests/openfl/experimental/workflow/NotebookTools/testcase_export_federated