STORM-2223: PMMLBolt #1816

hmcl · 2016-12-07T03:15:50Z

PMML Bolt supporting pluggable runners
JPMML runner implementation
Test Topology

vesense · 2016-12-07T03:57:33Z

Maybe the licenses between Apache Storm and JPMML(licensed under AGPLv3) are incompatible.
Accoding to https://www.apache.org/licenses/GPL-compatibility.html

GPLv3 software cannot be included in Apache projects.

HeartSaVioR · 2016-12-07T04:09:22Z

pmml-evaluator is AGPLv3.

http://search.maven.org/#artifactdetails%7Corg.jpmml%7Cpmml-evaluator%7C1.3.3%7Cjar

pmml-model and pmml-schema are, at least pom file license description, BSD.

But what's the difference between jpmml and pmml on artifact prefix, and why they share the group name?

hmcl · 2016-12-07T04:22:09Z

@vesense @HeartSaVioR this was discussed in the JIRA - please take a look at the discussion thread. I believe that the JPMML license (the legacy code) is OK as discussed there.

HeartSaVioR · 2016-12-07T04:36:54Z

@hmcl
Sorry I confused the version. Got it.

@vesense
Before 1.1 they're using Apache license.
https://github.com/jpmml/jpmml

vesense · 2016-12-07T07:08:49Z

@hmcl @HeartSaVioR Got it. Thanks.
But I wonder the legacy code(https://github.com/jpmml/jpmml) was updated three years ago and it seems would never been updated. This can be a problem.
Also, there has been a project(https://github.com/jpmml/jpmml-storm) for integrating with Storm.

harshach · 2016-12-07T07:17:56Z

@vesense there is no alternate option for PMML in java. This is atleast give us a good start .
If we see any adoption we are planning on adding further development to have our own java PMML library. Also the same version is used several other projects. Example cascading here https://github.com/Cascading/pattern/blob/wip-1.0/pattern-pmml/build.gradle#L26

regarding the jpmml-storm, I reached out the developer in his words he said that just an experiment code and not really prod quality and they have no interest in contributing to the storm.
Having several connectors and interesting NLP, ML bolts in storm itself will improve adoption of those connectors and will give users an incentive to use storm.
I don't see any issue having this in storm.

vesense · 2016-12-07T13:18:13Z

@harshach Thanks for your detailed explanations. Yes, this is a good start. I'm +1 for adding this in. This might attract many people who interest NLP, ML.

vesense · 2016-12-07T14:02:06Z

external/storm-pmml/src/main/java/org/apache/storm/pmml/runner/ModelRunner.java

+
+import java.util.List;
+
+public interface ModelRunner {


Since bolt can be executed in distributed environment, ModelRunner interface should extends Serializable.

vesense · 2016-12-07T14:08:56Z

@hmcl Code looks good. Left one comment. I think it is also necessary to add a README file to let people know how to use it.

hmcl · 2016-12-07T19:46:18Z

@vesense I addressed your serialization comment. I had to do some refactoring of the code because it was a strong requirement to enforce ModelRunner to be serializable. For instance, the JPMML implementation has several non serializable objects that really made this approach impossible. Rather I extract the output fields from the PMML model, and pass them to the bolt directly. The runner is then created in the prepare method. I will upload the README in bit.

@harshach @HeartSaVioR can you please take a look as well.

ptgoetz · 2016-12-07T20:07:11Z

Looks like a good start, but it really needs some documentation. It would also be helpful to include a sample model + CSV data, without that it's not very clear how to run the example.

ptgoetz · 2016-12-07T20:10:52Z

Also, what about unit tests?

csivaguru · 2016-12-07T20:56:20Z

external/storm-pmml/src/main/java/org/apache/storm/pmml/runner/jpmml/JpmmlFactory.java

+    /**
+     * Creates a new {@link PMML} object representing the PMML model defined in the XML {@link File} specified as argument
+     */
+    public static PMML newPmml(File file) throws JAXBException, SAXException, IOException {


Nit: Consider adding support for getting newPmml from an input stream as well.

It would also be very useful to be able to load/update models from the blob store so users don't have to redeploy a topology to update their model.

@ptgoetz I created STORM-2240 to address this as follow up

harshach · 2016-12-08T19:29:00Z

@hmcl I am +1 on merging. I would like to see the InputStream option added to it.

harshach · 2016-12-08T19:32:08Z

@hmcl you can add me as sponsor to the module.

csivaguru · 2016-12-08T21:50:50Z

external/storm-pmml/src/main/java/org/apache/storm/pmml/runner/jpmml/JPmmlModelRunner.java

+            rawInputs.put(activeField, tuple.getValueByField(activeField.getValue()));
+        }
+        LOG.debug("Raw inputs = [{}]", rawInputs);
+        return rawInputs;


Where is the streams being used?

good catch. removed it.

csivaguru · 2016-12-08T21:55:50Z

external/storm-pmml/src/main/java/org/apache/storm/pmml/runner/jpmml/JpmmlFactory.java

+    }
+
+
+    public static class ModelRunner implements ModelRunnerFactory {


Consider renaming, Having two ModelRunner artifacts (Class here) and the interface might be confusing.

hmcl · 2016-12-12T03:36:28Z

@csivaguru @ptgoetz @harshach I uploaded a reviewed version addressing your comments. Can you please take a look. thanks.

harshach · 2016-12-12T03:44:14Z

+1

csivaguru · 2016-12-12T21:18:18Z

+1

ptgoetz · 2016-12-13T19:12:36Z

external/storm-pmml/README.md

+
+#Run Bundled Examples
+
+To run the examples you must copy the `storm-pmml` uber jar to `STORM-HOME/extlib` and then run the command:


Can't the example just be a self contained shaded jar that is ready for deployment? It seems odd to require users to modify the pom and copy jars into extlib to run the example. Most of the other examples are deployable jars.

@ptgoetz do you mean that you would like to have an uber jar like storm-pmml-examples-2.0.0-SNAPSHOT.jar under external/storm-pmml such that the users could just do storm jar storm-pmml-examples-2.0.0-SNAPSHOT.jar org.apache.storm.pmm.TopologyMain` and run it ?

@ptgoetz users can now just run the examples Jar

ptgoetz · 2016-12-13T19:17:43Z

examples/storm-pmml-examples/src/main/resources/KNIME_PMML_4.1_Examples_single_audit_logreg.xml

@@ -0,0 +1,259 @@
+<?xml version="1.0" encoding="UTF-8"?>


Do we know what the license of this file is? It wasn't clear to me from the dmg.org website.

@ptgoetz I found PMML Notice and License.

Okay, it's BSD 3-clause. So there needs to be an entry in both source and binary distribution LICENSE files per this guide: http://www.apache.org/dev/licensing-howto.html#permissive-deps

ptgoetz · 2016-12-13T19:18:29Z

examples/storm-pmml-examples/src/main/resources/Audit.50.csv

@@ -0,0 +1,51 @@
+ID,Age,Employment,Education,Marital,Occupation,Income,Gender,Deductions,Hours,IGNORE_Accounts,RISK_Adjustment,TARGET_Adjusted


Also interested in what the license of the data set is.

ptgoetz · 2016-12-13T19:22:04Z

external/storm-pmml/README.md

+## Committer Sponsors
+ * Sriharsha Chintalapani ([sriharsha@apache.org](mailto:sriharsha@apache.org))
+
+This general abstraction has the purpose of supporting arbitrary implementations that compute predicted scores from raw inputs


Minor nit: This seems more appropriate for the introduction section.

Good catch. I forgot to clean this up. This info is already mentioned in the introduction section in slight different way, but with the same exact meaning. Will delete it.

arunmahadevan

Went through at a high level and left some comments.

arunmahadevan · 2016-12-14T05:59:57Z

examples/storm-pmml-examples/src/main/java/org/apache/storm/pmml/JpmmlRunnerTestTopology.java

+
+    private File loadExample(File file, String example) {
+        try (InputStream stream = Thread.currentThread().getContextClassLoader().getResourceAsStream(example)) {
+            file = File.createTempFile("/tmp/" + example, ".tmp");


the first argument is a prefix of the file name not a directory.

arunmahadevan · 2016-12-14T06:12:50Z

examples/storm-pmml-examples/src/main/java/org/apache/storm/pmml/RawInputFromCSVSpout.java

+
+    @Override
+    public void activate() {
+        openReader();


seems every time the spout is deactivated/activated it starts reading the file from the beginning which may not be desirable.

arunmahadevan · 2016-12-14T06:15:23Z

...lient/src/test/java/org/apache/storm/kafka/spout/test/KafkaSpoutTopologyMainNamedTopics.java

@@ -39,7 +39,6 @@
 import java.io.InputStreamReader;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.concurrent.TimeUnit;


unrelated file KafkaSpoutTopologyMainNamedTopics.java updated?

arunmahadevan · 2016-12-14T06:20:28Z

external/storm-pmml/src/main/java/org/apache/storm/pmml/PMMLPredictorBolt.java

+public class PMMLPredictorBolt extends BaseRichBolt {
+    protected static final Logger LOG = LoggerFactory.getLogger(PMMLPredictorBolt.class);
+
+    private ModelOutputFields outFields;


can be final

arunmahadevan · 2016-12-14T06:20:35Z

external/storm-pmml/src/main/java/org/apache/storm/pmml/PMMLPredictorBolt.java

+    protected static final Logger LOG = LoggerFactory.getLogger(PMMLPredictorBolt.class);
+
+    private ModelOutputFields outFields;
+    private ModelRunnerFactory runnerFactory;


can be final

arunmahadevan · 2016-12-14T06:32:50Z

external/storm-pmml/src/main/java/org/apache/storm/pmml/PMMLPredictorBolt.java

+
+    @Override
+    public void declareOutputFields(OutputFieldsDeclarer declarer) {
+        if (outFields != null) {


In the execute it seems the results are always emitted, wont it fail when outFields==null ?

Also how are you ensuring that the streams declared in declareStream here are the ones returned by runner.scoredTuplePerStream ?

I have documented this. The bottom line is that only the user knows which scores he wants to emit on each stream. If she/he chooses to emit on a stream that does not exist, that's a user error.

Also fixed outFields==null

arunmahadevan · 2016-12-14T06:44:13Z

external/storm-pmml/src/main/java/org/apache/storm/pmml/model/Stream.java

+/**
+ * Represents a stream emitted by a Storm component
+ */
+public class Stream implements Serializable {


It seems the Stream just wraps the stream name and the direct flag. However in the PMMLPredictorBolt emitDirect is not used at all. If all you want is to capture the stream name where the tuples have to be emitted just use String instead of wrapping it in another class.

Done. It's a String now.

arunmahadevan · 2016-12-14T06:46:43Z

external/storm-pmml/src/main/java/org/apache/storm/pmml/PMMLPredictorBolt.java

+            for (Map.Entry<? extends Stream, ? extends Fields> of : outFields.toMap().entrySet()) {
+                final Stream stream = of.getKey();
+                final Fields fields = of.getValue();
+                declarer.declareStream(stream.getId(), stream.isDirect(), fields);


doesnt make sense to declare stream as direct unless you intend to emit directly to specific tasks via emitDirect.

arunmahadevan · 2016-12-14T06:59:00Z

external/storm-pmml/src/main/java/org/apache/storm/pmml/runner/jpmml/JPmmlModelRunner.java

+
+        final Map<Stream, List<Object>> valuesMap = new HashMap<>();
+
+        for (Map.Entry<? extends Stream, ? extends Fields> entry : modelOutputFields.toMap().entrySet()) {


use keySet. It may be better if ModelOuputFields itself has a method to returns the keys, right now its more of map wrapper.

Done! Added a method to return all the streams, which is what we need here.

arunmahadevan · 2016-12-14T07:04:10Z

examples/storm-pmml-examples/src/main/java/org/apache/storm/pmml/JpmmlRunnerTestTopology.java

+import java.util.Arrays;
+import java.util.List;
+
+public class JpmmlRunnerTestTopology {


Maybe some short description of what this topology does/predicts.

hmcl · 2016-12-16T01:37:14Z

@ptgoetz @arunmahadevan @harshach Thanks for your reviews. I believe I addressed all the comments. There is a separate commit with the comments changes. When we all agree that the patch is ready to merge, I will squash the commits to keep the log clean. Thanks.

arunmahadevan · 2016-12-16T08:19:05Z

+1 the changes looks good. After unzipping the distribution, I am not able to find the examples jar file under examples/storm-pmml-examples. Its under external/storm-pmml.

hmcl · 2016-12-17T09:27:44Z

@arunmahadevan done!
@harshach @ptgoetz @csivaguru can you please take one final look.
If everything is OK, I will go ahead and squash the commits.
Thanks!

harshach · 2016-12-20T06:15:45Z

+1

harshach · 2016-12-20T06:15:54Z

@hmcl can you upmerge this.

harshach · 2016-12-20T06:16:37Z

@hmcl Also can you open a PR against 1.x-branch as well.

ptgoetz · 2016-12-20T19:54:21Z

storm-dist/binary/final-package/src/main/assembly/binary.xml

+                <include>storm*jar</include>
+            </includes>
+        </fileSet>
+


Do we want the sample model and data to be included in the distribution?

@ptgoetz done

ptgoetz · 2016-12-20T19:58:15Z

@hmcl Do you plan to add unit tests for this?

- PMML Bolt supporting pluggable runners and configurable stream output fields - JPMML runner implementation - JPMML Prediction Test Topology - README - LICENSE information

hmcl · 2016-12-21T22:27:56Z

@ptgoetz
The main focus of this initial patch is to design the classes in such a way that they can accommodate arbitrary runtime environments. The default implementation provided uses one such runtime execution library, which is more suited to be tested using integration tests. The integration tests are already provided through the example topology.

As for unit tests, I have filed this JIRA to assert for edge cases and some common cases.

harshach · 2016-12-22T19:05:45Z

@hmcl can you open a PR for 1.x-branch as well. I would like to merge this in today.
There is pending comment about license on the data file. Can you take a look.

hmcl · 2016-12-22T21:50:00Z

@harshach 1.x-branch PR.

@harshach I believe that the license issue was addressed in here

What I did was to follow the license guidelines as described in here,
which is to add the license entry to the src LICENSE
also for binary LICENSE

vesense reviewed Dec 7, 2016

View reviewed changes

hmcl force-pushed the Apache_master_STORM-2223_PMMLBolt branch from 6d6b712 to da61a8f Compare December 7, 2016 19:41

csivaguru reviewed Dec 7, 2016

View reviewed changes

csivaguru reviewed Dec 8, 2016

View reviewed changes

hmcl force-pushed the Apache_master_STORM-2223_PMMLBolt branch 2 times, most recently from 25d4464 to 0b78876 Compare December 12, 2016 03:06

ptgoetz reviewed Dec 13, 2016

View reviewed changes

arunmahadevan reviewed Dec 14, 2016

View reviewed changes

hmcl force-pushed the Apache_master_STORM-2223_PMMLBolt branch from 2fa294e to 39039d2 Compare December 17, 2016 09:25

hmcl mentioned this pull request Dec 20, 2016

Streamline-522 Add Model Registry hortonworks/streamline#384

Merged

hmcl force-pushed the Apache_master_STORM-2223_PMMLBolt branch 2 times, most recently from 3388be5 to 737f314 Compare December 20, 2016 19:13

ptgoetz reviewed Dec 20, 2016

View reviewed changes

hmcl force-pushed the Apache_master_STORM-2223_PMMLBolt branch from 737f314 to 87941a9 Compare December 21, 2016 01:11

STORM-2223: PMMLBolt

c85cc19

- PMML Bolt supporting pluggable runners and configurable stream output fields - JPMML runner implementation - JPMML Prediction Test Topology - README - LICENSE information

hmcl force-pushed the Apache_master_STORM-2223_PMMLBolt branch from 87941a9 to c85cc19 Compare December 21, 2016 22:17

hmcl mentioned this pull request Dec 22, 2016

STORM-2223: PMMLBolt #1838

Merged

asfgit merged commit c85cc19 into apache:master Dec 23, 2016

		}


		public static class ModelRunner implements ModelRunnerFactory {


		#Run Bundled Examples

		To run the examples you must copy the `storm-pmml` uber jar to `STORM-HOME/extlib` and then run the command:

		@@ -0,0 +1,51 @@
		ID,Age,Employment,Education,Marital,Occupation,Income,Gender,Deductions,Hours,IGNORE_Accounts,RISK_Adjustment,TARGET_Adjusted


		final Map<Stream, List<Object>> valuesMap = new HashMap<>();

		for (Map.Entry<? extends Stream, ? extends Fields> entry : modelOutputFields.toMap().entrySet()) {

STORM-2223: PMMLBolt #1816

STORM-2223: PMMLBolt #1816

Conversation

hmcl commented Dec 7, 2016

vesense commented Dec 7, 2016

HeartSaVioR commented Dec 7, 2016 • edited Loading

hmcl commented Dec 7, 2016

HeartSaVioR commented Dec 7, 2016

vesense commented Dec 7, 2016

harshach commented Dec 7, 2016

vesense commented Dec 7, 2016

Choose a reason for hiding this comment

vesense commented Dec 7, 2016

hmcl commented Dec 7, 2016

ptgoetz commented Dec 7, 2016

ptgoetz commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshach commented Dec 8, 2016

harshach commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmcl commented Dec 12, 2016

harshach commented Dec 12, 2016

csivaguru commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmcl Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunmahadevan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmcl commented Dec 16, 2016

arunmahadevan commented Dec 16, 2016

hmcl commented Dec 17, 2016

harshach commented Dec 20, 2016

harshach commented Dec 20, 2016

harshach commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptgoetz commented Dec 20, 2016

hmcl commented Dec 21, 2016

harshach commented Dec 22, 2016

hmcl commented Dec 22, 2016

HeartSaVioR commented Dec 7, 2016 •

edited

Loading

hmcl Dec 16, 2016 •

edited

Loading