MLeap Flavor/Deployment #1: Java Support by dbczumar · Pull Request #307 · mlflow/mlflow

dbczumar · 2018-08-15T20:22:00Z

This is the first PR in a multi-part series that will introduce support for the MLeap flavor in MLFlow. This PR provides the infrastructure for deserializing and invoking models as generic predictors in Java.

…rmat

…se to users

aarondav · 2018-08-16T01:57:38Z

java/src/main/java/com/databricks/mlflow/mleap/MLeapFlavor.java

+ * Represents an MLeap flavor configuration
+ */
+public class MLeapFlavor implements Flavor {
+    public static final String FLAVOR_NAME = "mleap";


Google's Java Style Guide would have us do 2-space indent for all Java files.

Formatted code using https://github.com/google/google-java-format

aarondav · 2018-08-16T01:58:20Z

java/src/main/java/com/databricks/mlflow/mleap/MLeapLoader.java

@@ -0,0 +1,50 @@
+package com.databricks.mlflow.mleap;


org.mlflow, everywhere

aarondav · 2018-08-16T01:58:57Z

java/src/main/java/com/databricks/mlflow/mleap/MLeapLoader.java

+import com.databricks.mlflow.sagemaker.PredictorLoadingException;
+import com.databricks.mlflow.utils.FileUtils;
+
+import java.util.Optional;


import order: all non-static imports in a single block

Formatted code using https://github.com/google/google-java-format

aarondav · 2018-08-16T02:01:48Z

java/src/test/java/com/databricks/mlflow/SerializationUtilsTests.java

+/**
+ * Unit tests for the {@link com.databricks.mlflow.utils.SerializationUtils} module
+ */
+public class SerializationUtilsTests {


Test names end with just "Test"

Updated all tests to end with Test instead of Tests.

aarondav · 2018-08-16T02:02:31Z

java/src/main/java/com/databricks/mlflow/models/Model.java

+     *
+     * @param modelRootPath The path to the root directory of the MLFlow model
+     */
+    public static Model fromRootPath(String modelRootPath) throws IOException {


Can we add a test for this method?

Added a test called ModelTests#testModelIsLoadedFromYamlUsingRootPathCorrectly

aarondav · 2018-08-16T02:03:13Z

java/src/test/java/com/databricks/mlflow/ModelTests.java

+            Assert.assertTrue(
+                model.getFlavor(MLeapFlavor.FLAVOR_NAME, MLeapFlavor.class).isPresent());
+            Assert.assertTrue(model.getUtcTimeCreated().isPresent());
+        } catch (IOException e) {


nit: I think you're allowed to add throws to a test, and it will fail the test if the exception is thrown. Maybe see if it works.

The exception handling is deliberate here, in order to provide additional information about the test failure. Added a stack trace dump to ensure that the original trace isn't lost.

aarondav · 2018-08-16T02:04:27Z

java/src/main/java/com/databricks/mlflow/utils/SerializationUtils.java

+     * @param class The class of the Java object that should be produced
+     */
+    public static <T> T parseJsonFromFile(String filePath, Class<T> objectClass)
+        throws IOException {


hypernit: continuation indents are at +4 indentation (compared to default +2) to distinguish from body

Formatted code using https://github.com/google/google-java-format

aarondav · 2018-08-16T02:05:39Z

java/src/main/java/com/databricks/mlflow/utils/SerializationUtils.java

+     * @param json A string in valid JSON format
+     */
+    public static <T> T fromJson(String json) throws IOException {
+        return jsonMapper.readValue(json, new TypeReference<T>() {});


I'm not sure I believe that this actually works for complex types. Do we need to define it, btw? Maybe we could remove the methods of SerializationUtils that are not currently used.

This is used later by MLeap. See https://github.com/mlflow/mlflow/pull/278/files#diff-71d9fc3d16e2e46961159e9efccdf573R52.

Good catch, this didn't work for complex types. Added a Class argument.

codecov-io · 2018-08-16T02:10:32Z

Codecov Report

Merging #307 into master will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
+ Coverage   55.81%   55.94%   +0.12%     
==========================================
  Files         108      108              
  Lines        5531     5554      +23     
==========================================
+ Hits         3087     3107      +20     
- Misses       2444     2447       +3

Impacted Files	Coverage Δ
mlflow/projects/__init__.py	`93.92% <0%> (+0.1%)`	⬆️
mlflow/tracking/__init__.py	`82.96% <0%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55828ab...4a421a5. Read the comment docs.

…ization utility

dbczumar

@aarondav Addressed your comments and added a mvn clean test line to our .travis.yml

dbczumar · 2018-08-16T18:17:26Z

java/src/main/java/com/databricks/mlflow/LoaderModule.java

+     * Loads an MLFlow model as a generic predictor that can be used for
+     * inference
+     *
+     * @throws {@link com.databricks.mlflow.sagemaker.PredictorLoadingException} if


Thanks! I've shortened links throughout the PR that correspond to imported modules.

dbczumar · 2018-08-16T18:17:34Z

java/src/main/java/com/databricks/mlflow/LoaderModule.java

+     * inference
+     *
+     * @throws {@link com.databricks.mlflow.sagemaker.PredictorLoadingException} if
+     * failures are encountered while attempting to load the model from the specified configuration.


Thanks for the inline replacement. Done!

dbczumar · 2018-08-16T18:20:05Z

java/src/main/java/com/databricks/mlflow/LoaderModule.java

+     */
+    public Predictor load(String modelRootPath) throws PredictorLoadingException {
+        try {
+            return load(Model.fromRootPath(modelRootPath));


Good call. Done!

dbczumar · 2018-08-16T18:32:54Z

java/src/main/java/com/databricks/mlflow/mleap/MLeapLoader.java

@@ -0,0 +1,50 @@
+package com.databricks.mlflow.mleap;


dbczumar · 2018-08-16T18:33:33Z

java/src/main/java/com/databricks/mlflow/models/Model.java

+     *
+     * @param modelRootPath The path to the root directory of the MLFlow model
+     */
+    public static Model fromRootPath(String modelRootPath) throws IOException {


Added a test called ModelTests#testModelIsLoadedFromYamlUsingRootPathCorrectly

dbczumar · 2018-08-16T18:38:23Z

java/src/test/java/com/databricks/mlflow/SerializationUtilsTests.java

+/**
+ * Unit tests for the {@link com.databricks.mlflow.utils.SerializationUtils} module
+ */
+public class SerializationUtilsTests {


Updated all tests to end with Test instead of Tests.

dbczumar · 2018-08-16T18:40:53Z

java/src/main/java/com/databricks/mlflow/utils/SerializationUtils.java

+     * @param json A string in valid JSON format
+     */
+    public static <T> T fromJson(String json) throws IOException {
+        return jsonMapper.readValue(json, new TypeReference<T>() {});


This is used later by MLeap. See https://github.com/mlflow/mlflow/pull/278/files#diff-71d9fc3d16e2e46961159e9efccdf573R52.

Good catch, this didn't work for complex types. Added a Class argument.

dbczumar · 2018-08-16T19:07:47Z

java/src/main/java/com/databricks/mlflow/utils/SerializationUtils.java

+     * @param class The class of the Java object that should be produced
+     */
+    public static <T> T parseJsonFromFile(String filePath, Class<T> objectClass)
+        throws IOException {


Formatted code using https://github.com/google/google-java-format

dbczumar · 2018-08-16T19:08:00Z

java/src/main/java/com/databricks/mlflow/mleap/MLeapFlavor.java

+ * Represents an MLeap flavor configuration
+ */
+public class MLeapFlavor implements Flavor {
+    public static final String FLAVOR_NAME = "mleap";


Formatted code using https://github.com/google/google-java-format

dbczumar · 2018-08-16T19:08:18Z

java/src/main/java/com/databricks/mlflow/mleap/MLeapLoader.java

+import com.databricks.mlflow.sagemaker.PredictorLoadingException;
+import com.databricks.mlflow.utils.FileUtils;
+
+import java.util.Optional;


Formatted code using https://github.com/google/google-java-format

aarondav · 2018-08-17T01:27:50Z

mlflow/java/src/main/java/org/mlflow/Flavor.java

@@ -0,0 +1,13 @@
+package org.mlflow;


Can we remove the .swn files? What are those? Should we add them to a gitignore?

aarondav · 2018-08-17T01:28:18Z

LGTM, just one comment

mateiz · 2018-08-17T04:50:34Z

mlflow/java/pom.xml

+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>org.mlflow</groupId>
+  <artifactId>mlflow-java</artifactId>


Kind of a nit but shouldn't we just call it mlflow? I guess we can rename it later too.

In the long run we'll probably want this to be a separate artifact from the Java API client anyway, so we'll need to make a more complicated pom.xml.

@mateiz Maven's package naming conventions, which are derived from Java conventions, indicate that a package name should correspond to a valid domain that the package owner controls. Since we own mlflow.org, org.mlflow seems to be the correct package name.

Here's the Java reference I found regarding package naming: https://docs.oracle.com/javase/specs/jls/se10/jls10.pdf (Page 137)

Ah, I wasn't asking about the org.mlflow line, but about the mlflow-java artifact ID. Why not just mlflow.

Ah, sorry! I'll correct this in the followup PR that adds SageMaker support to the Java library!

dbczumar added 30 commits July 26, 2018 17:31

Preliminary Java support

389d329

Renames

9f18043

Package restructuring

e66297d

Add mleap python flavor - preliminary

1dc4cca

Implement javafunc with correct loader module

e022261

Input parsing

5a9a24a

Add missing files

dc54bab

Failed attempt at package installation

80a2c8b

Fix up naming semantics, loader module impl

2914e02

Server impl

4606863

Fixes - serving works

35ff68a

Benchmarking results

2e12736

Benchmarking script

403cd10

SparkML critical path lats

23979f2

add batching to bmark

ac755a8

Remove runID from SageMakerServer

0aa09b7

..

04e7024

Add method that allows deserialization of MLeap pipeline in native fo…

b4fa632

…rmat

Move JavaFunc into protected sagemaker package

c8db3b0

Modify pyfunc container to handle mleap

e1323e6

Python API changes progress

ae2506e

Use package protection to hide modules that we don't yet wish to expo…

377cce7

…se to users

Fixes

0d98b45

Container mods

fbbcc3f

Remove mlflow-java directory

61af3e1

Serving is functional. Add sparkml module

4e6c2a5

Exception message fixes

e20f445

Throw unsupportedop from attempting to load a model from runId

9778713

Remove nonfunctional PackageInstaller

767be96

Package restructuring, remove unnecessary lines

de5cf91

aarondav reviewed Aug 16, 2018

View reviewed changes

dbczumar added 11 commits August 16, 2018 11:22

Doc link fixes

9f858a7

Change package name in java file

365e9b8

Change package name in directory structure

b94d2e2

Pom updates

1723dc9

Print stack trace on modeltest fail

3fc4c0f

Remove trailing from test case names

9282148

Rename tests to comply with style guide, add class argument to serial…

7634527

…ization utility

Format code

1e426fe

Format tests

0688e81

Rename to

97576b3

Update travis yml with Java tests

fefb324

dbczumar commented Aug 16, 2018

View reviewed changes

dbczumar added 2 commits August 16, 2018 12:20

Remove superfluous files

1a68025

Test fix

1e7a550

aarondav reviewed Aug 17, 2018

View reviewed changes

aarondav added the LGTM label Aug 17, 2018

mateiz reviewed Aug 17, 2018

View reviewed changes

dbczumar added 2 commits August 16, 2018 22:22

Remove swn files

9d7f823

Remove swm

4a421a5

dbczumar merged commit cfc9402 into mlflow:master Aug 17, 2018

Conversation

dbczumar commented Aug 15, 2018

Uh oh!

aarondav Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dbczumar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarondav Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarondav commented Aug 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbczumar Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarondav Aug 16, 2018 •

edited

Loading

codecov-io commented Aug 16, 2018 •

edited

Loading

aarondav Aug 17, 2018 •

edited

Loading

dbczumar Aug 17, 2018 •

edited

Loading