From dev-return-6239-archive-asf-public=cust-asf.ponee.io@madlib.apache.org Sat Oct 17 00:51:18 2020 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mxout1-ec2-va.apache.org (mxout1-ec2-va.apache.org [3.227.148.255]) by mx-eu-01.ponee.io (Postfix) with ESMTPS id D79F818037A for ; Sat, 17 Oct 2020 02:51:18 +0200 (CEST) Received: from mail.apache.org (mailroute1-lw-us.apache.org [207.244.88.153]) by mxout1-ec2-va.apache.org (ASF Mail Server at mxout1-ec2-va.apache.org) with SMTP id 204D84526D for ; Sat, 17 Oct 2020 00:51:18 +0000 (UTC) Received: (qmail 28784 invoked by uid 500); 17 Oct 2020 00:51:17 -0000 Mailing-List: contact dev-help@madlib.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@madlib.apache.org Delivered-To: mailing list dev@madlib.apache.org Received: (qmail 28772 invoked by uid 99); 17 Oct 2020 00:51:17 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 17 Oct 2020 00:51:17 +0000 From: =?utf-8?q?GitBox?= To: dev@madlib.apache.org Subject: =?utf-8?q?=5BGitHub=5D_=5Bmadlib=5D_kaknikhil_commented_on_a_change_in_pull_?= =?utf-8?q?request_=23522=3A_DL=3A_Remove_keras_dependency?= Message-ID: <160289587743.32230.9889823226213289034.asfpy@gitbox.apache.org> Date: Sat, 17 Oct 2020 00:51:17 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit In-Reply-To: References: 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