Add Scala 2.12 and 2.13 cross-compilation (#16438) #17503

fomkin · 2020-02-01T15:34:40Z

Description

Currently, MXNet supports only ancient Scala 2.11. This prevents using the library in the Scala community. I found mentions of try to add support of Scala 2.12 but looks like the trial was failed, although PR was merged. This PR is another try.

Comments

API compatibility was completely saved
Add scala-2.11, scala-2.12 and scala-2.13 Maven profiles. Now one of the profiles should be active. This condition is checked by maven-enforcer-plugin
Add Support of Scala 2.13 collections to C++ JNI code
Module "Examples" builds only for 2.11. Dependencies were obsolete and there is no way to port examples without rewriting of logic (this is out of scope of the PR)
Module "Spark ML" builds only for 2.11. It depends on Spark 1.6.3 which supports only 2.11
Possible performance degradation in 2.13 version. Scala 2.13 has new collections library. Default Seq is immutable now, so I need to ivoke toSeq for each ArrayBuffer to copy it to immutable sequence

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

To solve

I did something and BertQA starts to compile (looks like it was ignored before). It breaks compilation of examples. I don't know what to do. I'm nood in neural networks and not know how no modify the code to make it compile.
Makefile and CI integration.
Test it. Can't run test for now Tests failed when I try to build scala-package from source #17483 :(

leezu · 2020-02-01T21:30:33Z

Just FYI, there are currently some issues with the CI. So not every failure of the tests you observe is due to your changes.

I suggest you also take a look at #16167 (comment) . For example, the Module "Examples" you mention above would likely be rendered obsolete as part of MXNet 2.

In terms of frontend, this roadmap focuses mostly on Python-frontend since MXNet has been taking a Python-first approach. The expectation with respect to other language bindings is that they would evolve along with the backend evolution and make use of the improvements. Given that breaking changes can occur, maintainers of different language bindings are expected to participate in related interface definition discussions.

fomkin · 2020-02-10T07:50:54Z

Examples do not compile because I've updated scala-maven-plugin to the latest version. It fixes some cases where *.java files are ignoring during compilation. May I remove examples from the build? It will fix CI.

leezu · 2020-02-10T16:59:20Z

Please CC the persons who contributed the respective example (see git history of the file) in this issue.

zachgk

@fomkin Removing the examples may fix the CI now, but it will be bad for the project down the line as those examples would no longer be maintained. It is odd that those examples were not working. I looked at the current problem. In BertQA.java lines 77 and 79, try to use null as the second argument to the softmaxParam constructor. If this doesn't work or other examples are broken, I can help fix them or you can ask whoever created the example (see git blame).

@gigasquid Would this affect clojure at all?

zachgk · 2020-02-12T22:15:56Z

scala-package/assembly/pom.xml

@@ -26,7 +26,7 @@
    <relativePath>../pom.xml</relativePath>
  </parent>

-  <artifactId>mxnet-full_2.11</artifactId>
+  <artifactId>mxnet-full</artifactId>


Is there a reason you are removing the _{scala.binary.version} from the internal artifactId? This could end up breaking other users

This is an artifactId in the Maven reactor scope. Concrete Scala version suffix adds on deploy phase (see deploy/pom.xml). It means that artifact published to the Central will have proper Scala version suffix. So it shouldn't break other users.

When install scala-package locally, ~/.m2/repository will contain artifact without suffix. I sure that it advanced usage option, and user know what he or she doing. At least he or she explicitly sets profile with Scala version.

zachgk · 2020-02-12T22:20:30Z

scala-package/core/src/test/scala/org/apache/mxnet/NDArraySuite.scala

        .flatMap(x => Array.fill[Float](repeat)(x.toFloat))
      val range2 = NDArray.arange(stop, step = step, repeat = repeat)
      assert(CheckUtils.reldiff(result2.toArray, range2.toArray) <= 1e-4f)

-      val result3 = 0f to stop by 1f
+      val result3 = (BigDecimal(0) until BigDecimal(stop) by BigDecimal(1)).map(_.toFloat)


BigDecimal(0) to BigDecimal(stop)?

zachgk · 2020-02-12T22:32:56Z

scala-package/pom.xml

@@ -423,23 +590,11 @@
      <version>1.7.7</version>
      <scope>provided</scope>
    </dependency>
-    <dependency>
-      <groupId>org.scalatest</groupId>
-      <artifactId>scalatest_2.11</artifactId>


Can you try to keep the dependencies in the dependency section so it is easier to keep track of them. For scalatest, you could just use <artifactId>scalatest_${scala.binary.version}</artifactId>. If you need to vary the version, you could use a property.

I tried to do this. But it doesn't working. Looks like maven-remote-resources-plugin doesn't understand profile-depended properties in dependencies. Here is error log https://gist.github.com/fomkin/a26aefc59eaca1148dfed3719a8144f0

Well. It solves by removing default properties: maven-remote-resources-plugin ignores the property from a profile if it has default values.

fomkin · 2020-02-13T08:12:16Z

@zachgk

In BertQA.java lines 77 and 79, try to use null as the second argument to the softmaxParam constructor.

Thanks. Now it compiles.

fomkin · 2020-02-13T11:13:49Z

But throws exception 🤕

* Add Maven profiles * Prepare FFI to work with Scala 2.13 collections * Modify code to support Scala 2.13 * Modify CI scripts to run test with all Scala versions

gigasquid · 2020-02-13T15:03:23Z

@fomkin Removing the examples may fix the CI now, but it will be bad for the project down the line as those examples would no longer be maintained. It is odd that those examples were not working. I looked at the current problem. In BertQA.java lines 77 and 79, try to use null as the second argument to the softmaxParam constructor. If this doesn't work or other examples are broken, I can help fix them or you can ask whoever created the example (see git blame).

@gigasquid Would this affect clojure at all?

No - That shouldn't affect Clojure. Thanks for thinking about us though :)

fomkin · 2020-02-13T15:48:20Z

@gigasquid May you look at this http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-cpu/detail/PR-17503/7/pipeline#step-600-log-1655 ? What I had do wrong?

fomkin · 2020-02-13T15:56:10Z

@lanking520 may you look at BertQA.java? The code use obsolete API. It never compiled in CI before because of bug in scala-maven-plugin. Now it compiles and breaks the build. I tried to fix this giving same NDArray as length to softmaxParam. It became compilable but tests fails with the error: http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-gpu/detail/PR-17503/7/pipeline#step-804-log-1708

gigasquid · 2020-02-13T16:05:26Z

@gigasquid May you look at this http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-cpu/detail/PR-17503/7/pipeline#step-600-log-1655 ? What I had do wrong?

I stand corrected. It does affect the Clojure package as well. A library we use depends on the old version of scala and would need to be forked/upgraded to work https://github.com/t6/from-scala/blob/master/project.clj

TaoLv · 2020-02-13T16:06:44Z

You may also want to set use_length=True? See the API definition: https://mxnet.incubator.apache.org/api/python/docs/api/symbol/symbol.html#mxnet.symbol.softmax

fomkin · 2020-02-13T16:17:31Z

@gigasquid Looks like it simpler to downgrade Scala 2.11 back to 2.11.8. Upgrading minor Scala version is not the purpose of this PR.

zachgk · 2020-02-13T19:05:06Z

You may also want to set use_length=True? See the API definition: https://mxnet.incubator.apache.org/api/python/docs/api/symbol/symbol.html#mxnet.symbol.softmax

@TaoLv Is something wrong with the softmax operator? It says that the length argument is required, but I think it is supposed to be optional (and used to be optional). The examples in the documentation you linked to don't include a length argument. The current problem is that one of our java examples originally called softmax(data), but now the signature is softmax(data, length) so we need to figure out what to pass for the length argument.

zachgk requested review from lanking520 and zachgk February 12, 2020 22:50

zachgk reviewed Feb 12, 2020

View reviewed changes

Add Scala 2.12 and 2.13 cross-compilation (apache#16438)

6ca47c6

* Add Maven profiles * Prepare FFI to work with Scala 2.13 collections * Modify code to support Scala 2.13 * Modify CI scripts to run test with all Scala versions

szha added the v1.x Targeting v1.x branch label Aug 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Scala 2.12 and 2.13 cross-compilation (#16438) #17503

Add Scala 2.12 and 2.13 cross-compilation (#16438) #17503

fomkin commented Feb 1, 2020 •

edited

Loading

leezu commented Feb 1, 2020

fomkin commented Feb 10, 2020

leezu commented Feb 10, 2020

zachgk left a comment

zachgk Feb 12, 2020

fomkin Feb 13, 2020 •

edited

Loading

fomkin Feb 13, 2020

zachgk Feb 12, 2020

fomkin Feb 13, 2020

zachgk Feb 12, 2020

fomkin Feb 13, 2020 •

edited

Loading

fomkin Feb 13, 2020

fomkin commented Feb 13, 2020

fomkin commented Feb 13, 2020

gigasquid commented Feb 13, 2020

fomkin commented Feb 13, 2020

fomkin commented Feb 13, 2020

gigasquid commented Feb 13, 2020

TaoLv commented Feb 13, 2020

fomkin commented Feb 13, 2020 •

edited

Loading

zachgk commented Feb 13, 2020

Add Scala 2.12 and 2.13 cross-compilation (#16438) #17503

Are you sure you want to change the base?

Add Scala 2.12 and 2.13 cross-compilation (#16438) #17503

Conversation

fomkin commented Feb 1, 2020 • edited Loading

Description

Comments

Checklist

Essentials

To solve

leezu commented Feb 1, 2020

fomkin commented Feb 10, 2020

leezu commented Feb 10, 2020

zachgk left a comment

Choose a reason for hiding this comment

zachgk Feb 12, 2020

Choose a reason for hiding this comment

fomkin Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

fomkin Feb 13, 2020

Choose a reason for hiding this comment

zachgk Feb 12, 2020

Choose a reason for hiding this comment

fomkin Feb 13, 2020

Choose a reason for hiding this comment

zachgk Feb 12, 2020

Choose a reason for hiding this comment

fomkin Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

fomkin Feb 13, 2020

Choose a reason for hiding this comment

fomkin commented Feb 13, 2020

fomkin commented Feb 13, 2020

gigasquid commented Feb 13, 2020

fomkin commented Feb 13, 2020

fomkin commented Feb 13, 2020

gigasquid commented Feb 13, 2020

TaoLv commented Feb 13, 2020

fomkin commented Feb 13, 2020 • edited Loading

zachgk commented Feb 13, 2020

fomkin commented Feb 1, 2020 •

edited

Loading

fomkin Feb 13, 2020 •

edited

Loading

fomkin Feb 13, 2020 •

edited

Loading

fomkin commented Feb 13, 2020 •

edited

Loading