[v1.0.22] Code review fixes (#18)

* Code review fixes

* Code review fixes

* Removes redundant line

* workflow fix

* workflow fix

* workflow fix

* workflow fix

* bug fix

.github/workflows/sdk-test.yml

@@ -2,9 +2,7 @@ name: SDK Tests
 on:
   push:
     paths:
-        - 'sdk'
-    branches-ignore:
-      - main
+        - 'sdk/**'
 defaults:
   run:
     working-directory: sdk

sdk/.flake8

@@ -1,7 +1,7 @@
 [flake8]
 select = ANN,B,B9,BLK,C,D,DAR,E,F,I,S,W
 ignore = E203,E501,W503,ANN002,ANN003,ANN101,D100,D104,DAR401,S104,C901
-max-line-length = 120
+max-line-length = 100
 max-complexity = 10
 application-import-names = mlnotify,tests
 import-order-style = google

sdk/.pre-commit-config.yaml

@@ -9,25 +9,25 @@ repos:
     hooks:
     -   id: isort
         name: sort imports
-        entry: python "./sdk/scripts/run_script.py" "poetry run isort --filter-files ./"
+        entry: python "./sdk/scripts/run_precommit_hook.py" "poetry run isort --filter-files ./"
         language: system
         types: [python]
         pass_filenames: false
     -   id: black
         name: black
-        entry: python "./sdk/scripts/run_script.py" "poetry run python -m black ./"
+        entry: python "./sdk/scripts/run_precommit_hook.py" "poetry run python -m black ./"
         language: system
         pass_filenames: false
         types: [python]
     -   id: flake8
         name: flake8
-        entry: python "./sdk/scripts/run_script.py" "poetry run flake8 ./"
+        entry: python "./sdk/scripts/run_precommit_hook.py" "poetry run flake8 ./"
         language: system
         types: [python]
         pass_filenames: false
     -   id: mypy
         name: mypy
-        entry: python "./sdk/scripts/run_script.py" "poetry run mypy ./"
+        entry: python "./sdk/scripts/run_precommit_hook.py" "poetry run mypy ./"
         language: system
         types: [python]
         pass_filenames: false

sdk/pyproject.toml

@@ -2,15 +2,15 @@
 name = "mlnotify"
 version = "1.0.0"
 description = "ML Notify - A useful tool that notifies you when your model is finished training"
-authors = []
-readme = "README.md"
+authors = ['Aporia']
 repository = "https://github.com/aporia-ai/mlnotify"
 
 [tool.poetry.dependencies]
 python = "^3.6"
 gorilla = "^0.4.0"
 qrcode = "^6.1"
 requests = "^2.25.1"
+dataclasses = { version = "0.8", python = "<3.7" }
 
 [tool.poetry.dev-dependencies]
 pytest = "^5.3.2"
@@ -37,7 +37,7 @@ lexicographical = true
 order_by_type = false
 group_by_package = true
 no_lines_before = ['LOCALFOLDER']
-line_length = 120
+line_length = 100
 
 [build-system]
 requires = ["poetry-core>=1.0.0"]

sdk/src/mlnotify/__init__.py

@@ -1,4 +1,7 @@
-from mlnotify.mlnotify import end, plugins_manager, start  # type: ignore # noqa: F401
+from mlnotify.mlnotify import plugin_manager
 from mlnotify.plugins.base import BasePlugin
 
-__all__ = [start, end, plugins_manager, BasePlugin]
+start = plugin_manager.run_before
+end = plugin_manager.run_after
+
+__all__ = [start, end, plugin_manager, BasePlugin]

sdk/src/mlnotify/hooks.py

@@ -6,49 +6,47 @@
 
 from mlnotify.logger import logger
 
-HookFunction = Callable
-
 GORILLA_SETTINGS = gorilla.Settings(allow_hit=True, store_hit=True)
 
 
-def base_patch_func(*args, __original_func: Callable, __before: HookFunction, __after: HookFunction, **kwargs) -> Any:
+def base_patch_func(*args, __original_func: Callable, __before: Callable, __after: Callable, **kwargs) -> Any:
     """When patching a method, this function will run instead.
 
     Args:
-        __original_func (Callable): The original function before the patch
-        __before (HookFunction): The function to run before the original function
-        __after (HookFunction): The function to run after the original function
+        __original_func: The original function before the patch
+        __before: The function to run before the original function
+        __after: The function to run after the original function
         *args: positional args passed for the original function
         **kwargs: keyword args passed for the original function
 
     Returns:
-        result (Any): The original function's result
+        result: The original function's result
     """
     logger.debug("Running patched func", args, kwargs)
 
     try:
         __before()
-    except Exception as e:
-        logger.debug("Failed to run hook function (before)", e)
+    except Exception:
+        logger.debug("Failed to run hook function (before)", exc_info=True)
 
     res = __original_func(*args, **kwargs)
 
     try:
         __after()
-    except Exception as e:
-        logger.debug("Failed to run hook function (after)", e)
+    except Exception:
+        logger.debug("Failed to run hook function (after)", exc_info=True)
 
     return res
 
 
-def patch(destination: Any, name: str, before: HookFunction, after: HookFunction) -> None:
+def patch(destination: Any, name: str, before: Callable, after: Callable) -> None:
     """This function patches the destination.name function and replaces it with the base_patch_func.
 
     Args:
-        destination (Any): The class/dict to patch
-        name (str): The function name to be replaced
-        before (HookFunction): The function to run before the original function
-        after (HookFunction): The function to run after the original function
+        destination: The class/dict to patch
+        name: The function name to be replaced
+        before: The function to run before the original function
+        after: The function to run after the original function
     """
     original_func: Callable = gorilla.get_attribute(destination, name)
 
@@ -59,49 +57,56 @@ def patch(destination: Any, name: str, before: HookFunction, after: HookFunction
     gorilla.apply(patch)
 
 
-def apply_hooks(before: HookFunction, after: HookFunction):
-    """Applys hooks.
+def apply_hooks(before: Callable, after: Callable):
+    """Applies hooks.
 
     This function applies all hooks - imports the relevant packages and patches
     the specified functions. Since usually not all packages exist, it runs on
     a best-effort assumption.
 
     Args:
-        before (HookFunction): The function to run before the original function
-        after (HookFunction): The function to run after the original function
+        before: The function to run before the original function
+        after: The function to run after the original function
     """
     logger.debug("Applying hooks")
 
+    # LightGBM
     try:
         import lightgbm
 
         patch(lightgbm, "train", before=before, after=after)
         patch(lightgbm.sklearn, "train", before=before, after=after)
-    except Exception as e:
-        logger.debug("Could not import and patch lightgbm", e)
+    except Exception:
+        logger.debug("Could not import and patch lightgbm", exc_info=True)
+
+    # XGBoost
     try:
         import xgboost
 
         patch(xgboost, "train", before=before, after=after)
         patch(xgboost.sklearn, "train", before=before, after=after)
-    except Exception as e:
-        logger.debug("Could not import and patch xgboost", e)
+    except Exception:
+        logger.debug("Could not import and patch xgboost", exc_info=True)
+
+    # Tensorflow & Keras
     try:
         import tensorflow
 
         patch(tensorflow.keras.Model, "fit", before=before, after=after)
         patch(tensorflow.keras.Model, "train_on_batch", before=before, after=after)
-    except Exception as e:
-        logger.debug("Could not import and patch tensorflow.keras", e)
+    except Exception:
+        logger.debug("Could not import and patch tensorflow.keras", exc_info=True)
         # If tensorflow.keras patching doesn't work, we can try
         # patching keras as a standalone
         try:
             import keras
 
             patch(keras.Model, "fit", before=before, after=after)
             patch(keras.Model, "train_on_batch", before=before, after=after)
-        except Exception as e:
-            logger.debug("Could not import and patch keras", e)
+        except Exception:
+            logger.debug("Could not import and patch keras", exc_info=True)
+
+    # SKLearn
     try:
         import sklearn.svm
         import sklearn.tree
@@ -115,5 +120,5 @@ def apply_hooks(before: HookFunction, after: HookFunction):
         patch(sklearn.svm.LinearSVC, "fit", before=before, after=after)
         patch(sklearn.tree.DecisionTreeClassifier, "fit", before=before, after=after)
         patch(sklearn.tree.DecisionTreeRegressor, "fit", before=before, after=after)
-    except ImportError as e:
-        logger.debug("Could not import and patch sklearn", e)
+    except ImportError:
+        logger.debug("Could not import and patch sklearn", exc_info=True)

sdk/src/mlnotify/logger.py

@@ -3,23 +3,28 @@
 
 LOGGER_NAME = "mlnotify"
 
-logging.config.dictConfig(
-    {
-        "version": 1,
-        "handlers": {
-            "console": {
-                "class": "logging.StreamHandler",
-                "level": "WARNING",
-                "formatter": "standard",
-                "stream": "ext://sys.stdout",
-            }
-        },
-        "formatters": {"standard": {"format": "%(asctime)s [%(name)s] %(levelname)s: %(message)s"}},
-        LOGGER_NAME: {"level": "WARNING", "handlers": ["console"]},
-        "disable_existing_loggers": False,
-    }
-)
 
-logger = logging.getLogger(LOGGER_NAME)
+def init_logger():
+    logging.config.dictConfig(
+        {
+            "version": 1,
+            "handlers": {
+                "console": {
+                    "class": "logging.StreamHandler",
+                    "level": "WARNING",
+                    "formatter": "standard",
+                    "stream": "ext://sys.stdout",
+                }
+            },
+            "formatters": {"standard": {"format": "%(asctime)s [%(name)s] %(levelname)s: %(message)s"}},
+            LOGGER_NAME: {"level": "WARNING", "handlers": ["console"]},
+            "disable_existing_loggers": False,
+        }
+    )
+    logger = logging.getLogger(LOGGER_NAME)
 
-logger.debug(f"Logger is set to level {logger.level}")
+    logger.debug(f"Logger level is set to {logger.level}")
+    return logger
+
+
+logger = init_logger()

sdk/src/mlnotify/mlnotify.py

@@ -1,11 +1,18 @@
 from mlnotify.hooks import apply_hooks
+from mlnotify.plugin_manager import PluginManager
 from mlnotify.plugins.notify import NotifyPlugin
-from mlnotify.plugins_manager import PluginsManager
 
-plugins_manager = PluginsManager()
-plugins_manager.register_plugin(NotifyPlugin())
 
-apply_hooks(before=plugins_manager.run_before, after=plugins_manager.run_after)
+def init_plugin_manager():
+    """
+    Initialize the plugin manager.
+    """
+    plugin_manager = PluginManager()
+    plugin_manager.register_plugin(NotifyPlugin())
 
-start = plugins_manager.run_before
-end = plugins_manager.run_after
+    apply_hooks(before=plugin_manager.run_before, after=plugin_manager.run_after)
+
+    return plugin_manager
+
+
+plugin_manager = init_plugin_manager()

sdk/src/mlnotify/plugins_manager.py → sdk/src/mlnotify/plugin_manager.py

@@ -4,20 +4,21 @@
 from mlnotify.plugins.base import BasePlugin
 
 
-class PluginsManager:
+class PluginManager:
     """Manages the plugins.
 
-    The PluginsManager allows registering and clearing plugins,
+    The PluginManager allows registering and clearing plugins,
     as well as invoking their hooks (before, after) serially.
     """
 
-    plugins: List[BasePlugin] = []
+    def __init__(self) -> None:
+        self.plugins: List[BasePlugin] = []
 
     def register_plugin(self, plugin: BasePlugin):
         """Adds a plugin.
 
         Args:
-            plugin (BasePlugin): The plugin to add
+            plugin: The plugin to add
         """
         self.plugins.append(plugin)
 
@@ -33,8 +34,8 @@ def run_before(self):
             if hasattr(plugin, "before"):
                 try:
                     plugin.before()
-                except Exception as e:
-                    print(f"Failed to run a plugin's `before` function [{plugin}]: {e}")
+                except Exception:
+                    logger.error(f"Failed to run a plugin's `before` function [{plugin}]", exc_info=True)
 
     def run_after(self):
         """Runs all registered plugins' after function."""
@@ -44,5 +45,5 @@ def run_after(self):
             if hasattr(plugin, "after"):
                 try:
                     plugin.after()
-                except Exception as e:
-                    print(f"Failed to run a plugin's `after` function [{plugin}]: {e}")
+                except Exception:
+                    logger.error(f"Failed to run a plugin's `after` function [{plugin}]", exc_info=True)