[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1816 @harshach 1.x-branch [PR](https://github.com/apache/storm/pull/1838). @harshach I believe that the license issue was addressed in [here](https://github.com/apache/storm/pull/1816#pullrequestreview-12756266) What I did was to follow the license guidelines as described in [here](http://www.apache.org/dev/licensing-howto.html#permissive-deps), which is to add the license entry to the src [LICENSE](https://github.com/apache/storm/pull/1816/files#diff-9879d6db96fd29134fc802214163b95a) also for binary [LICENSE](https://github.com/apache/storm/pull/1816/files#diff-8d914c320178e25abc06496e0fb9fc8b) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user harshach commented on the issue: https://github.com/apache/storm/pull/1816 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1816 @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](https://issues.apache.org/jira/browse/STORM-2253) to assert for edge cases and some common cases. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1816 @hmcl Do you plan to add unit tests for this? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user harshach commented on the issue: https://github.com/apache/storm/pull/1816 @hmcl Also can you open a PR against 1.x-branch as well. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user harshach commented on the issue: https://github.com/apache/storm/pull/1816 @hmcl can you upmerge this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user harshach commented on the issue: https://github.com/apache/storm/pull/1816 +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1816 @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! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/1816 +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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1816 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user csivaguru commented on the issue: https://github.com/apache/storm/pull/1816 +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user harshach commented on the issue: https://github.com/apache/storm/pull/1816 +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1816 @csivaguru @ptgoetz @harshach I uploaded a reviewed version addressing your comments. Can you please take a look. thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user harshach commented on the issue: https://github.com/apache/storm/pull/1816 @hmcl you can add me as sponsor to the module. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user harshach commented on the issue: https://github.com/apache/storm/pull/1816 @hmcl I am +1 on merging. I would like to see the InputStream option added to it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1816 Also, what about unit tests? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1816 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1816 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1816 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1816 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user harshach commented on the issue: https://github.com/apache/storm/pull/1816 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1816 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1816 @hmcl Sorry I confused the version. Got it. @vesense Before 1.1 they're using Apache license. https://github.com/jpmml/jpmml --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1816 @vesense @HeartSaVioR this was discussed in the [JIRA](https://issues.apache.org/jira/browse/STORM-2223) - please take a look at the discussion thread. I believe that the [JPMML](https://github.com/jpmml/jpmml) license (the legacy code) is OK as discussed there. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1816 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, and why they share the group name? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1816: STORM-2223: PMMLBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1816 hi @hmcl 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
