[GitHub] [madlib] kaknikhil commented on a change in pull request #522: DL: Remove keras dependency

[GitBox <gi...@apache.org>]

kaknikhil commented on a change in pull request #522:
URL: https://github.com/apache/madlib/pull/522#discussion_r505089686



##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_wrapper.py_in
##########
@@ -248,7 +249,7 @@ def parse_optimizer(compile_dict):
     opt_split = compile_dict['optimizer'].split('(')
     opt_name = opt_split[0]
     optimizers = get_optimizers()
-    _assert(opt_name in optimizers,
+    _assert(opt_name.lower() in [o.lower() for o in optimizers.keys()],

Review comment:
       what if opt_name is None ?

##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras.py_in
##########
@@ -44,7 +39,15 @@ from utilities.utilities import madlib_version
 from utilities.utilities import unique_string
 from utilities.validate_args import get_expr_type
 from utilities.control import MinWarning
+
 import tensorflow as tf
+from tensorflow import keras

Review comment:
       We weren't explicitly importing keras before, is this for the case when user has a `keras` dependency in their model json ?

##########
File path: src/ports/postgres/modules/deep_learning/test/madlib_keras_fit.sql_in
##########
@@ -397,36 +397,3 @@ SELECT assert(
     class_values = '{NULL,0,1,4,5}',
     'Keras model output Summary Validation failed. Actual:' || __to_char(summary))
 FROM (SELECT * FROM keras_saved_out_summary) summary;
-

Review comment:
       I think it will help to add a note to the commit message explaining why we removed these tests 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org