reductionista commented on a change in pull request #560:
URL: https://github.com/apache/madlib/pull/560#discussion_r590934148
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_wrapper.py_in
##########
@@ -440,16 +444,16 @@ def get_custom_functions_list(compile_params):
"""
compile_dict = convert_string_of_args_to_dict(compile_params)
- builtin_losses = dir(losses)
+ builtin_losses = update_builtin_losses(dir(losses))
builtin_metrics = update_builtin_metrics(dir(metrics))
custom_fn_list = []
local_loss = compile_dict['loss'].lower() if 'loss' in compile_dict else
None
- local_metric = compile_dict['metrics'].lower()[2:-2] if 'metrics' in
compile_dict else None
+ metric_list = ast.literal_eval(compile_dict['metrics'].lower()) if
'metrics' in compile_dict else []
Review comment:
I think it's better to catch the problem on master, than send bad input
to the segments and let them find out. If we just use the [2,-2] method then
we may label some inputs as builtin functions when they aren't. So if it
successfully literal evals later, it would get passed all the way to keras even
if it doesn't match any builtin functions or compile functions.
I think it's best we interpret the string they pass in the same way each
time we process it. If it's handled one way here, and then another way later,
then there are more possibilities for how the user can insert quotes and other
special characters to confuse MADlib.
However, looking at this again I notice there is a try/catch block around
the literal_eval that happens on the segments. If it errors out here, it won't
give as nice of an error message... so maybe we should add a similar try/catch
around it?
----------------------------------------------------------------
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:
us...@infra.apache.org
