feat: local registry #421

GustavBaumgart · 2023-05-25T17:40:24Z

Description

A local registry saves the metrics (which are currently implemented on a per-round basis) to a ~/flame-log directory. The structure of this directory is flame-log --> job_id --> task_id --> {model, params, metrics}. Each metric is saved in its own separate csv.

Additionally, the method signatures for call were changed, and the method signatures for save_params were aligned to what the MLflow registry has used.

Type of Change

Checklist

I have read the contributing guidelines
Existing issues have been referenced (where applicable)
I have verified this change is not present in other open pull requests
Functionality is documented
All code style checks pass
New code contribution is covered by automated tests
All new and existing tests pass

codecov-commenter · 2023-05-25T17:46:48Z

Codecov Report

Merging #421 (202f798) into main (53a850e) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main     #421   +/-   ##
=======================================
  Coverage   14.86%   14.86%           
=======================================
  Files          48       48           
  Lines        2832     2832           
=======================================
  Hits          421      421           
  Misses       2382     2382           
  Partials       29       29

myungjin

left comments.

lib/python/flame/config.py

myungjin · 2023-05-26T01:21:39Z

lib/python/flame/mode/distributed/trainer.py

@@ -70,7 +70,7 @@ def internal_init(self) -> None:
        """Initialize internal state for role."""
        self.registry_client = registry_provider.get(self.config.registry.sort)
        # initialize registry client
-        self.registry_client(self.config.registry.uri, self.config.job.job_id)
+        self.registry_client(self.config.registry.uri, self.config)


Passing self.config alone may be sufficient since registry.uri can be accessed from config.

myungjin · 2023-05-26T01:21:51Z

lib/python/flame/mode/horizontal/syncfl/top_aggregator.py

@@ -81,7 +81,7 @@ def internal_init(self) -> None:

        self.registry_client = registry_provider.get(self.config.registry.sort)
        # initialize registry client
-        self.registry_client(self.config.registry.uri, self.config.job.job_id)
+        self.registry_client(self.config.registry.uri, self.config)


myungjin · 2023-05-26T01:22:03Z

lib/python/flame/mode/horizontal/syncfl/trainer.py

@@ -71,7 +71,7 @@ def internal_init(self) -> None:

        self.registry_client = registry_provider.get(self.config.registry.sort)
        # initialize registry client
-        self.registry_client(self.config.registry.uri, self.config.job.job_id)
+        self.registry_client(self.config.registry.uri, self.config)


myungjin · 2023-05-26T01:22:44Z

lib/python/flame/registries.py

@@ -21,6 +21,7 @@
 from .object_factory import ObjectFactory
 from .registry.dummy import DummyRegistryClient
 from .registry.mlflow import MLflowRegistryClient


please refactor other imports to use absolute path

myungjin · 2023-05-26T01:30:58Z

lib/python/flame/registry/local.py

+    def setup_run(self, name: str) -> None:
+        """Set up a run."""
+        # set up directories
+        home_dir = os.path.expanduser("~")


Perhaps it may be better to use pathlib?
An example usage:

from pathlib import Path print(Path.home())

myungjin · 2023-05-26T01:34:56Z

lib/python/flame/registry/local.py

+        log_dir = os.path.join(home_dir, "flame-log")
+        self.registry_path = os.path.join(log_dir, self.job_id, self.task_id)
+
+        os.system(f'rm -rf {self.registry_path}')


perhaps better to use shutil.rmtree()?

myungjin · 2023-05-26T01:37:53Z

lib/python/flame/registry/mlflow.py

@@ -23,7 +23,7 @@
 from mlflow.exceptions import MlflowException
 from mlflow.protos.databricks_pb2 import RESOURCE_ALREADY_EXISTS, ErrorCode

-from ..config import Hyperparameters
+from ..config import (Hyperparameters, Config)
 from .abstract import AbstractRegistryClient


please use absolute path

myungjin · 2023-05-26T01:41:43Z

lib/python/flame/registry/local.py

+        os.mkdir(os.path.join(self.registry_path, "metrics"))
+        os.mkdir(os.path.join(self.registry_path, "model"))
+        os.mkdir(os.path.join(self.registry_path, "params"))
+        #os.mkdir(os.path.join(self.registry_path, "artifact"))


remove it if this is not needed

myungjin · 2023-05-26T01:43:40Z

lib/python/flame/registry/local.py

+        os.makedirs(self.registry_path)
+        os.mkdir(os.path.join(self.registry_path, "metrics"))
+        os.mkdir(os.path.join(self.registry_path, "model"))
+        os.mkdir(os.path.join(self.registry_path, "params"))


Instead of lines 57-60, how about the following?

for dir in ["metrics", "model", "params"]: os.makedirs(os.path.join(self.registry_path, dir))

jaemin-shin

Left some comments. Agree to Myungjin's comments too!

jaemin-shin · 2023-05-26T19:17:43Z

lib/python/flame/registry/local.py

+        self.model_versions = defaultdict(lambda: 1)
+
+    def save_metrics(self, epoch: int, metrics: Optional[dict[str,
+                                                              float]]) -> None:


better merge line 68 with 67. Setting up formatter would help

jaemin-shin · 2023-05-26T19:22:58Z

lib/python/flame/registry/local.py

+        """Save a model in a model registry."""
+        model_folder = os.path.join(self.registry_path, "model", name)
+        if not os.path.exists(model_folder):
+            os.mkdir(model_folder)


Suggested change

os.mkdir(model_folder)

os.makedirs(model_folder, exist_ok=True)

You may consider merging 95 and 96 into one line like this

jaemin-shin · 2023-05-26T19:26:17Z

lib/python/flame/registry/local.py

+            import tensorflow
+            return tensorflow.keras.models.load_model(model_path)
+
+        return None


how about we raise an error like ModuleNotFoundError, instead of return None?

myungjin

left a few comments; please consider to address them

myungjin · 2023-05-31T00:24:28Z

lib/python/flame/registry/local.py

+        if os.path.exists(self.registry_path):
+            shutil.rmtree(self.registry_path)
+
+        os.makedirs(self.registry_path)


This doesn't seem necessary because line 65 will create any intermediate directories in the path.

myungjin · 2023-05-31T00:33:40Z

lib/python/flame/registry/local.py

+            shutil.rmtree(self.registry_path)
+
+        os.makedirs(self.registry_path)
+        for directory in ["metrics", "model", "params"]:


These ("metrics", "model" and "params") are used in several places in the code.
In this case, making them as constants may be better to reduce the chance of introducing a bug.

myungjin · 2023-05-31T00:48:31Z

lib/python/flame/registry/local.py

+                with open(filename, "w") as file:
+                    csv_writer = csv.writer(file)
+                    csv_writer.writerow(["round", metric])
+                    csv_writer.writerow([epoch, metrics[metric]])


I think lines 74-83 can be simplified in the following way:

exists = os.path.exists(filename): with open(filename, "a+") as file: csv_writer = csv.writer(file) csv_writer.writerow(["round", metric]) if not exists else None csv_writer.writerow([epoch, metrics[metric]])

myungjin

left a few comments; please consider to address them

jaemin-shin

LGTM for me! Left one minor comment.

jaemin-shin · 2023-05-31T01:04:30Z

lib/python/flame/registry/local.py

+        if ml_framework == MLFramework.PYTORCH:
+            import torch
+
+            # is not safe but matches behavior of MLFlow registry


what part of the code does this comment indicates? could be confusing as there are same comment at different location in line 124. Also, the comment itself could be more detailed to be helpful, by explaining which behavior of MLFlow registry it refers to

A local registry saves the metrics (which are currently implemented on a per-round basis) to a ~/flame-log directory. The structure of this directory is flame-log --> job_id --> task_id --> {model, params, metrics}. Each metric is saved in its own separate csv. Additionally, the method signatures for __call__ were changed, and the method signatures for save_params were aligned to what the MLFlow registry has used. mlflow_runname was moved from flame/common/util.py to flame/registry/mlflow.py.

myungjin

lgtm

GustavBaumgart requested review from myungjin and jaemin-shin May 25, 2023 17:40

myungjin reviewed May 26, 2023

View reviewed changes

jaemin-shin reviewed May 26, 2023

View reviewed changes

GustavBaumgart force-pushed the registry branch 2 times, most recently from 202f798 to f640cce Compare May 27, 2023 01:03

GustavBaumgart marked this pull request as ready for review May 30, 2023 20:59

GustavBaumgart requested review from jaemin-shin and myungjin May 30, 2023 21:00

myungjin reviewed May 31, 2023

View reviewed changes

jaemin-shin reviewed May 31, 2023

View reviewed changes

GustavBaumgart force-pushed the registry branch from f640cce to 361ec22 Compare May 31, 2023 17:36

myungjin approved these changes May 31, 2023

View reviewed changes

GustavBaumgart merged commit 6ffabf8 into cisco-open:main May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: local registry #421

feat: local registry #421

GustavBaumgart commented May 25, 2023

codecov-commenter commented May 25, 2023 •

edited

Loading

myungjin left a comment

myungjin May 26, 2023

myungjin May 26, 2023

myungjin May 26, 2023

myungjin May 26, 2023

myungjin May 26, 2023

myungjin May 26, 2023

myungjin May 26, 2023

myungjin May 26, 2023

myungjin May 26, 2023

jaemin-shin left a comment

jaemin-shin May 26, 2023

jaemin-shin May 26, 2023

jaemin-shin May 26, 2023

myungjin left a comment

myungjin May 31, 2023

myungjin May 31, 2023

myungjin May 31, 2023

myungjin left a comment

jaemin-shin left a comment

jaemin-shin May 31, 2023

myungjin left a comment

	os.mkdir(model_folder)
	os.makedirs(model_folder, exist_ok=True)

feat: local registry #421

feat: local registry #421

Conversation

GustavBaumgart commented May 25, 2023

Description

Type of Change

Checklist

codecov-commenter commented May 25, 2023 • edited Loading

Codecov Report

myungjin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaemin-shin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myungjin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myungjin left a comment

Choose a reason for hiding this comment

jaemin-shin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myungjin left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 25, 2023 •

edited

Loading