----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36016/#review90072 -----------------------------------------------------------
lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/BaseSparkClassificationModel.java (line 64) <https://reviews.apache.org/r/36016/#comment143018> Can we add which feature name is missing to exception message? Instead of failing at the first missing feature, we should find all missing ones and throw an exception at the end insted. lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/SparkMLDriver.java (line 53) <https://reviews.apache.org/r/36016/#comment143019> Add better javadoc for newly createad variables. lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/kmeans/KMeansAlgo.java (line 167) <https://reviews.apache.org/r/36016/#comment143021> Why is this returning null? lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/kmeans/KMeansAlgo.java (line 172) <https://reviews.apache.org/r/36016/#comment143022> same as above lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/nb/NaiveBayesAlgo.java (line 80) <https://reviews.apache.org/r/36016/#comment143023> Is fully qualified class name for AlgoParam required? I see getParams implemented with exact same implementation in many places. Could we move this to a parent class or made into some utility method? lens-ml-lib/src/main/java/org/apache/lens/ml/api/DataSet.java (line 26) <https://reviews.apache.org/r/36016/#comment143024> DataSet should have database name as well. Thougts? lens-ml-lib/src/main/java/org/apache/lens/ml/api/DataSet.java (line 38) <https://reviews.apache.org/r/36016/#comment143026> Try to use lombok annotations to be consistent with rest of the Lens code. There are annotations for getters, setters and a @data annotation as well. Also check out @AllArgsConstructor. This applies to all JAXB classes introduced in this patch. lens-ml-lib/src/main/java/org/apache/lens/ml/api/Status.java (line 32) <https://reviews.apache.org/r/36016/#comment143028> Would there be a CANCELLED or KILLED status? lens-ml-lib/src/main/java/org/apache/lens/ml/dao/InMemoryMetaStoreClient.java (line 31) <https://reviews.apache.org/r/36016/#comment143032> When are items evicted from these maps? Is it addition only? lens-ml-lib/src/main/java/org/apache/lens/ml/dao/InMemoryMetaStoreClient.java (line 77) <https://reviews.apache.org/r/36016/#comment143029> ? lens-ml-lib/src/main/java/org/apache/lens/ml/dao/MetaStoreClient.java (line 34) <https://reviews.apache.org/r/36016/#comment143033> Can we add a database param here? lens-ml-lib/src/main/java/org/apache/lens/ml/dao/MetaStoreClient.java (line 72) <https://reviews.apache.org/r/36016/#comment143030> Why is there a string return type expected here? In InMemory implementation I see the same ID of the argument is returned. That can be avoided. I think void return type with a declared exception to indicate failure should be done here. lens-ml-lib/src/main/java/org/apache/lens/ml/dao/MetaStoreClient.java (line 84) <https://reviews.apache.org/r/36016/#comment143031> What is returned for non existing model instances? Should it throw some exception instead of returning null? lens-ml-lib/src/main/java/org/apache/lens/ml/impl/HiveMLUDF.java (line 90) <https://reviews.apache.org/r/36016/#comment143034> Error message does not match arg count. lens-ml-lib/src/main/java/org/apache/lens/ml/impl/HiveMLUDF.java (line 97) <https://reviews.apache.org/r/36016/#comment143035> This error message needs rethought. Error message should explicitly call out missing values (or keys for that matter) Indicating there should be odd number of parameters might confuse user. We should point out exactly what is wrong. lens-ml-lib/src/main/java/org/apache/lens/ml/impl/HiveMLUDF.java (line 112) <https://reviews.apache.org/r/36016/#comment143040> Since evaluate is called for every single row in he input, we need to reduce some of the logging here, especially if the message is not going to change between rows. lens-ml-lib/src/main/java/org/apache/lens/ml/impl/HiveMLUDF.java (line 125) <https://reviews.apache.org/r/36016/#comment143039> Should be debug level log here. lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 75) <https://reviews.apache.org/r/36016/#comment143045> Retain @Slf4j annotation lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 280) <https://reviews.apache.org/r/36016/#comment143050> should be trainingResult.toString instead? why valueof? lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 448) <https://reviews.apache.org/r/36016/#comment143058> Set thread name lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 458) <https://reviews.apache.org/r/36016/#comment143051> Should not start threads in init(), use a separate start() method for that. lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 507) <https://reviews.apache.org/r/36016/#comment143052> Can we use a single thread executor here? Why are we creating threads directly? lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 518) <https://reviews.apache.org/r/36016/#comment143055> This thread reference is lost? Who is joining on this? Is it possible to use a bounded thread pool here and reuse threads? lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 522) <https://reviews.apache.org/r/36016/#comment143053> should be LOG.error lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 621) <https://reviews.apache.org/r/36016/#comment143056> Convert to log message lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 634) <https://reviews.apache.org/r/36016/#comment143057> Use lombok annotations here as well as in other context classes. lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 902) <https://reviews.apache.org/r/36016/#comment143060> Same as above. Please consider using executors. lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 907) <https://reviews.apache.org/r/36016/#comment143059> LOG.error lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java (line 1008) <https://reviews.apache.org/r/36016/#comment143062> Convert to log message. - Jaideep dhok On July 1, 2015, 9:52 a.m., vikas singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36016/ > ----------------------------------------------------------- > > (Updated July 1, 2015, 9:52 a.m.) > > > Review request for lens, Amareshwari Sriramadasu and sharad agarwal. > > > Bugs: LENS-581 > https://issues.apache.org/jira/browse/LENS-581 > > > Repository: lens > > > Description > ------- > > Implementation for new Lens ML API. This is not a complete implementation. > Model creation, traning into modelInstances. evaluations and prediction is > implemented. > Each of the modelInstance, prediciton and evaluation is creation in an async > manner. Metastore and Web API is not implemented. Testing of the code is done. > > > Diffs > ----- > > lens-ml-lib/data/lr/lr_train.data PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/client/LensMLClient.java 6dd0ecf > lens-ml-lib/src/main/java/org/apache/lens/client/LensMLJerseyClient.java > c68dd12 > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/api/AlgoParam.java > e0d13c0 > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/api/Algorithm.java > 29bde29 > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/api/MLAlgo.java 65373c6 > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/api/MLDriver.java d2a2748 > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/api/MLModel.java 73717ac > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/lib/AlgoArgParser.java > 00f20fc > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/lib/Algorithms.java > ad37403 > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/lib/ClassifierBaseModel.java > a960a4a > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/lib/ForecastingModel.java > 16a6180 > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/sdk/Algorithm.java > PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/sdk/MLDriver.java > PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/sdk/TrainedModel.java > PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/BaseSparkAlgo.java > 86cab38 > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/BaseSparkClassificationModel.java > 806dc1f > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/ColumnFeatureFunction.java > d75efc0 > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/HiveTableRDD.java > 4960e1e > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/SparkMLDriver.java > 21ed87d > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/TableTrainingSpec.java > 897aacb > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/dt/DecisionTreeAlgo.java > 7810a9a > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/dt/DecisionTreeClassificationModel.java > 27c32f4 > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/dt/SparkDecisionTreeModel.java > e561a8d > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/kmeans/KMeansAlgo.java > be9af18 > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/kmeans/KMeansClusteringModel.java > 62dc536 > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/lr/LogisticRegressionAlgo.java > c2f97af > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/lr/LogitRegressionClassificationModel.java > a4206e5 > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/nb/NaiveBayesAlgo.java > c484dfe > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/nb/NaiveBayesClassificationModel.java > 26d39df > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/svm/SVMAlgo.java > 4b14d66 > > lens-ml-lib/src/main/java/org/apache/lens/ml/algo/spark/svm/SVMClassificationModel.java > 433c0f9 > lens-ml-lib/src/main/java/org/apache/lens/ml/api/Algo.java PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/api/AlgoParam.java > PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/api/AlgoSpec.java PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/api/DataSet.java PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/api/Evaluation.java > PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/api/Feature.java PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/api/LensML.java 23b5437 > lens-ml-lib/src/main/java/org/apache/lens/ml/api/MLTestReport.java 965161a > lens-ml-lib/src/main/java/org/apache/lens/ml/api/Model.java PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/api/ModelInstance.java > PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/api/ModelMetadata.java 3f7dff1 > lens-ml-lib/src/main/java/org/apache/lens/ml/api/Prediction.java > PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/api/Status.java PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/api/TestReport.java 294fef3 > > lens-ml-lib/src/main/java/org/apache/lens/ml/dao/InMemoryMetaStoreClient.java > PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/dao/MLDBUtils.java d444a32 > lens-ml-lib/src/main/java/org/apache/lens/ml/dao/MetaStoreClient.java > PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/impl/BatchPredictSpec.java > PRE-CREATION > lens-ml-lib/src/main/java/org/apache/lens/ml/impl/HiveMLUDF.java 60a4008 > lens-ml-lib/src/main/java/org/apache/lens/ml/impl/LensMLImpl.java 5938f8a > lens-ml-lib/src/main/java/org/apache/lens/ml/impl/MLRunner.java e5ec272 > lens-ml-lib/src/main/java/org/apache/lens/ml/impl/MLTask.java 2867b90 > lens-ml-lib/src/main/java/org/apache/lens/ml/impl/MLUtils.java 9c96d9b > lens-ml-lib/src/main/java/org/apache/lens/ml/impl/ModelLoader.java c0e7953 > lens-ml-lib/src/main/java/org/apache/lens/ml/impl/QueryRunner.java 2eeee29 > lens-ml-lib/src/main/java/org/apache/lens/ml/impl/TableTestingSpec.java > 34b2a3f > lens-ml-lib/src/main/java/org/apache/lens/ml/server/MLApp.java e6e3c02 > lens-ml-lib/src/main/java/org/apache/lens/ml/server/MLServiceImpl.java > de9c711 > lens-ml-lib/src/main/java/org/apache/lens/ml/server/MLServiceResource.java > 77a6c40 > lens-ml-lib/src/main/java/org/apache/lens/rdd/LensRDDClient.java fc4164f > lens-ml-lib/src/test/java/org/apache/lens/ml/ExampleUtils.java 9fe1ea0 > lens-ml-lib/src/test/java/org/apache/lens/ml/TestMLResource.java 51344ce > lens-ml-lib/src/test/java/org/apache/lens/ml/TestMLRunner.java 7f6413b > lens-ml-lib/src/test/java/org/apache/lens/ml/TestMLServices.java > PRE-CREATION > lens-ml-lib/src/test/resources/lens-site.xml 3d5dbef > lens-ml-lib/src/test/resources/log4j.properties PRE-CREATION > src/site/apt/user/cli.apt 353c171 > > Diff: https://reviews.apache.org/r/36016/diff/ > > > Testing > ------- > > yes. > > > Thanks, > > vikas singh > >
