[SPARK-14613][ML] Add @Since into the matrix and vector classes in spark-mllib-local #12416

pravingadakh · 2016-04-15T11:59:01Z

What changes were proposed in this pull request?

This PR adds @since tag into the matrix and vector classes in spark-mllib-local.

How was this patch tested?

Scala-style checks passed.

srowen · 2016-04-15T12:01:53Z

mllib-local/src/main/scala/org/apache/spark/ml/linalg/Matrices.scala

@@ -759,6 +769,8 @@ object SparseMatrix {

 /**
 * Factory methods for [[org.apache.spark.ml.linalg.Matrix]].
+ *
+ * @since 2.0


These need to be annotations not scaladoc, like @Since("2.0.0")

@srowen Jira description says that these can not be annotations:

In spark-mllib-local, we're no longer to be able to use @Since annotation. As a result, we will switch to standard java doc style using /* @Since /*. This task will add those new APIs as @Since 2.0

Not sure of reasoning behind it though.

Oh right, because that's actually an annotation in Spark Core and it's not available. My mistake. For now fair enough then. But do write 2.0.0

dbtsai · 2016-04-15T17:17:00Z

ok to test

dbtsai · 2016-04-15T17:20:36Z

All the public methods need to have Since for the doc. See the following for reference. Thanks.

https://github.com/apache/spark/blob/master/mllib/src/main/scala/org/apache/spark/mllib/linalg/Vectors.scala

dbtsai · 2016-04-15T17:23:39Z

@srowen I think using scala annotation is easier, and I like it more. What do you think that we have a copy of @Since annotation in mllib-local jar? Thanks.

SparkQA · 2016-04-15T17:55:36Z

Test build #55938 has finished for PR 12416 at commit 4f4f789.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

holdenk · 2016-04-15T18:33:27Z

I think either moving the annotation to a common base (like sketch) or copying it would be a good idea.

srowen · 2016-04-15T18:50:12Z

Yeah, really it doesn't belong there though. It belongs in some super-core project behind all of the projects behind core. For one class, maybe not worth it; if there are many others, maybe so.

holdenk · 2016-04-15T18:52:18Z

@srowen Maybe not sketch, we have some testing annotations in common/tags - we could extend that beyond just testing tags to include the documentation tags we want to access?

MLnick · 2016-04-18T08:04:23Z

common/tags has a compile-time dependency on scalatest, will that not bring in that dep if used in mllib-local, or core?

srowen · 2016-04-18T08:12:31Z

If common/tags is supposed to be an annotation module, that's a good place. However, the current annotations under src/main are all actually test-related. Ideally these would move under src/test and dependencies would be updated to properly reference test scope everywhere. Then this could be part of src/main

dbtsai · 2016-04-18T17:29:48Z

I like @srowen 's idea. Having the shared annotation in common/tags, and move the current ones under src/test. @pravingadakh, can you update this PR? Thanks.

holdenk · 2016-04-18T17:52:43Z

@pravingadakh let me know if you need any help :)

pravingadakh · 2016-04-19T10:54:31Z

@dbtsai I'll update the PR accordingly. I just have one doubt before making any further changes, do you want to make a copy of @Since under common/tags or should I move @Since annotation from core to common/tags? @holdenk I'll definitely contact you if I am stuck anywhere :)

srowen · 2016-04-19T11:44:24Z

You would move the annotation. There's no sense in having two copies. Tagging @rxin since he made the tags module I believe.

rxin · 2016-04-19T17:33:04Z

yea just move it.

pravingadakh · 2016-04-20T16:11:57Z

@dbtsai @srowen Please validate the changes.

SparkQA · 2016-04-20T16:21:38Z

Test build #56373 has finished for PR 12416 at commit e617dd9.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class DenseMatrix @Since(\"2.0.0\") (
- class SparseMatrix @Since(\"2.0.0\") (
- class DenseVector @Since(\"2.0.0\") (@Since(\"2.0.0\") val values: Array[Double]) extends Vector
- class SparseVector @Since(\"2.0.0\") (

srowen · 2016-04-20T16:52:00Z

common/tags/pom.xml

+            <goals><goal>add-source</goal></goals>
+            <configuration>
+              <sources>
+                <source>src/main/java</source>


Is this actually required? the scala compiler plugin should already know all of these source locations

It is only required by src/test/java/, but thought I should include all source directories.

What requires it? I wonder if this isn't actually a problem that needs to be solved by the classifier declarations below.

We moved existing test annotations under common/tags from src/main package to src/test package, thus those annotation imports will not work as is, since by default src/test will be treated as test sources directory unless we explicitly mark it as sources directory.

The plugin already knows about src/test though. I think your problem is something else, like what's below or this: you have a problem with the new directory structure. They're now under a directory called "org.apache.spark.tags" instead of nested dirs "org/apache/spark/tags"

SparkQA · 2016-04-20T18:17:20Z

Test build #56378 has finished for PR 12416 at commit 05b755b.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

dbtsai · 2016-04-22T15:31:51Z

Ping @pravingadakh for update. Thanks.

pravingadakh · 2016-04-23T05:41:05Z

@dbtsai I'll update the PR soon, I have been overwhelmed by office work :(

SparkQA · 2016-04-23T09:08:04Z

Test build #56795 has finished for PR 12416 at commit 714afac.

This patch fails MiMa tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2016-04-23T11:24:56Z

Test build #56796 has finished for PR 12416 at commit 1da5e91.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-04-23T11:31:41Z

mllib-local/src/main/scala/org/apache/spark/ml/linalg/Matrices.scala

-    val values: Array[Double],
-    override val isTransposed: Boolean) extends Matrix {
+@Since("2.0.0")
+class DenseMatrix @Since("2.0.0") (


This might be overkill, since the class is marked "since 2.0.0". Everything in it is implicitly "since 2.0.0" as well -- or later. But right now there's no later version.

@srowen Yeah it does look like a overkill, I was just referring this class.

srowen · 2016-04-23T11:34:52Z

This looks correct to my eyes. The Since annotation is in place as a non-test class; the existing test classes are moved into the test source root. The parent pom now declares available dependencies on the test or non-test artifacts from this module. Everything refers to the new module, and most dependencies are non-test (which implicitly includes the test artifacts) because they need @Since directly. I agree with renaming the module too. This LGTM as far as I know, except possibly the question of whether the class constructors are over-annotated (CC @mengxr). I wouldn't mind another look from @dbtsai or @vanzin or @JoshRosen.

srowen · 2016-04-25T11:07:16Z

@pravingadakh sorry this needs a rebase now

pravingadakh · 2016-04-25T15:11:32Z

@srowen Done rebasing.

vanzin · 2016-04-25T17:26:22Z

I was going to ask why do we need an annotation for this (instead of javadoc's @since), but since that's already used in other places in the code, it's kind of a moot point.

SparkQA · 2016-04-28T22:33:33Z

Test build #57273 has finished for PR 12416 at commit f81616e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-28T22:59:43Z

Test build #57276 has finished for PR 12416 at commit 8b57dc0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dbtsai · 2016-04-28T23:00:21Z

Thanks. Merged into master.

yhuai · 2016-04-29T02:49:52Z

Looks like this one breaks the pr builder? https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57289/testReport/org.apache.spark.network/ChunkFetchIntegrationSuite/ChunkFetchIntegrationSuite/

yhuai · 2016-04-29T02:53:37Z

Sorry. I am going to revert it. I believe it breaks the build. Seems those build changes are not related to adding Since tag.

yhuai · 2016-04-29T02:58:24Z

I have reverted this commit.

srowen · 2016-04-29T10:53:56Z

Hm, is the issue that the SBT build won't see these classes now that they're test-scope? they are properly test scope though. If really necessary we could, I suppose, bring them back into compile scope, but that would be unfortunate. Does anyone know enough SBT to see how to make it see the test classes?

yhuai · 2016-04-29T15:01:48Z

Seems we are moving ExtendedYarnTest to src/test, but we are not making others depend on the test jar? Also, can we separate the build change with adding Since tag to those ML methods? I am not sure they should be done in a single PR.

yhuai · 2016-04-29T15:03:43Z

In sql/hive's pom, we have

<dependency>
      <groupId>org.apache.spark</groupId>
      <artifactId>spark-sql_${scala.binary.version}</artifactId>
      <type>test-jar</type>
      <version>${project.version}</version>
      <scope>test</scope>
    </dependency>

I am wondering if we have to specify <type>test-jar</type>?

srowen · 2016-04-29T15:27:31Z

Yeah these are separable. Really, adding the annotation requires the build change, and the latter grew to dominate the change.

Hm, I had suggested using <classifier>tests</classifier> to depend on test artifacts. I wonder if that's not the standard way to do it? it's how I've expressed this in the past. But I also see examples of <type>test-jar</type> @vanzin you probably know this better than I. However I don't know if that's the issue here.

This particular failure is turning up from lots of tests, that are in modules with nothing to do with the test tags. Was the last change to declare all these 'tags' dependencies the problem? most modules have nothing to do with it. They're just annotations... they don't matter to any logic.

vanzin · 2016-04-29T17:08:36Z

Seems we are moving ExtendedYarnTest to src/test, but we are not making others depend on the test jar?

Haven't looked in detail (and missed this in my not-so-careful read of the original patch), but that could be a problem, yes. The test tags need to be in every project's test classpath.

Regarding type vs. classifier, I believe classifier is the new recommendation, although both generally work the same for tests.

pravingadakh · 2016-04-29T17:26:53Z

@srowen I undid the changes done in dev/sparktestsupport/modules.py, ran the sbt test build locally with build/sbt -Pyarn -Phadoop-2.3 -Phive -Pkinesis-asl -Phive-thriftserver -Dtest.exclude.tags=org.apache.spark.tags.ExtendedHiveTest,org.apache.spark.tags.ExtendedYarnTest test command, all tests passed except one from graphx/test. The graphx test failed is

[info] - Label Propagation *** FAILED *** (5 seconds, 618 milliseconds)
[info] org.apache.spark.SparkException: Job aborted due to stage failure: Failed to serialize task 65, not attempting to retry it. Exception during serialization: java.lang.UnsupportedOperationException: Accumulator must be registered before send to executor
[info] at org.apache.spark.scheduler.DAGScheduler.org$apache$spark$scheduler$DAGScheduler$$failJobAndIndependentStages(DAGScheduler.scala:1448)

which I believe has nothing to do with python module changes (maybe I do not have updated code in my branch). So yes, I think build failure is due to changes (related to tags) done in dev/sparktestsupport/modules.py.

dbtsai · 2016-04-29T17:54:26Z

@pravingadakh The changes in modules.py is just helping Jenkins to understand the dependencies, and I don't get it why this will break the build.

Is there any way to reproduce it locally? Why the original Jenkins build worked?

vanzin · 2016-04-29T17:58:12Z

common/network-common/pom.xml

@@ -66,7 +66,7 @@
    </dependency>
    <dependency>
      <groupId>org.apache.spark</groupId>
-      <artifactId>spark-test-tags_${scala.binary.version}</artifactId>
+      <artifactId>spark-tags_${scala.binary.version}</artifactId>


Basically, what you have to do is: everywhere you're changing this, you need to also add <classifier>tests</classifier>. In modules where both the test and non-test tags are used, you need both dependencies (with and without classifier).

It might be easier to just leave the test tags in the main source base...

pravingadakh · 2016-04-29T18:26:17Z

@dbtsai @srowen My mistake, apparently changes in modules.py does not cause build failure. And I was unable to reproduce the locally (even after rebasing with latest master). Although I see difference in sbt test build command ran in both the builds.

Passed build sbt command:

[info] Running Spark tests using SBT with these arguments: -Pyarn -Phadoop-2.3 -Phive -Pkinesis-asl -Phive-thriftserver test

Failed build sbt command:

[info] Running Spark tests using SBT with these arguments: -Pyarn -Phadoop-2.3 -Phive -Pkinesis-asl -Phive-thriftserver -Dtest.exclude.tags=org.apache.spark.tags.ExtendedHiveTest,org.apache.spark.tags.ExtendedYarnTest test

But both command works (weird) for me locally.

srowen · 2016-04-30T15:11:03Z

OK, let's try this PR again but only with the tag changes first. (You're even welcome to make a separate JIRA that blocks this one, since that much is a fairly separable change.)

I vote for just leaving these tags in src/main then. Although they're logically test-only, they're just annotations, so can plausibly stay there I guess. I think we still want to rename the module, ideally. It's just that we won't make the MLlib changes and won't move the annotations this time, and so won't need any classifier or type changes in Maven.

Thanks @pravingadakh , my suggestion ended up being the wrong one here.

pravingadakh · 2016-05-02T18:32:56Z

OK, let's try this PR again but only with the tag changes first.

@srowen I'm little confused here, could you please elaborate a bit here? What tag changes specifically?
Thanks.

srowen · 2016-05-02T18:34:10Z

Meaning, let's leave aside the actual original change in the PR, which is to annotate some methods as @Since a release. Let's make the structural change to move the annotation first and rename the module.

pravingadakh · 2016-05-02T18:36:59Z

Aah ok, got it thanks. I'll create new jira for that and make that a blocker for this one.

srowen · 2016-05-06T13:58:39Z

@pravingadakh go ahead - would be good to get this in for 2.0

srowen · 2016-05-11T10:08:43Z

@pravingadakh if you're busy, I may resubmit this change on your behalf to try to get it in for 2.0

pravingadakh · 2016-05-12T06:51:38Z

@srowen Yes, please go ahead. Thanks.

srowen · 2016-05-12T09:17:07Z

See #13074 for the module change. I'm also moving other common annotations

…nto spark-tags ## What changes were proposed in this pull request? (See #12416 where most of this was already reviewed and committed; this is just the module structure and move part. This change does not move the annotations into test scope, which was the apparently problem last time.) Rename `spark-test-tags` -> `spark-tags`; move common annotations like `Since` to `spark-tags` ## How was this patch tested? Jenkins tests. Author: Sean Owen <sowen@cloudera.com> Closes #13074 from srowen/SPARK-15290. (cherry picked from commit 122302c) Signed-off-by: Sean Owen <sowen@cloudera.com>

…nto spark-tags ## What changes were proposed in this pull request? (See #12416 where most of this was already reviewed and committed; this is just the module structure and move part. This change does not move the annotations into test scope, which was the apparently problem last time.) Rename `spark-test-tags` -> `spark-tags`; move common annotations like `Since` to `spark-tags` ## How was this patch tested? Jenkins tests. Author: Sean Owen <sowen@cloudera.com> Closes #13074 from srowen/SPARK-15290.

srowen reviewed Apr 15, 2016
View reviewed changes

srowen reviewed Apr 20, 2016
View reviewed changes

srowen reviewed Apr 23, 2016
View reviewed changes

pravingadakh force-pushed the SPARK-14613 branch from 1da5e91 to 9b36881 Compare April 25, 2016 15:10

asfgit closed this in dae538a Apr 28, 2016

vanzin reviewed Apr 29, 2016
View reviewed changes

pravingadakh deleted the SPARK-14613 branch May 2, 2016 18:37

srowen mentioned this pull request May 12, 2016

[SPARK-15290] [BUILD] Move annotations, like @Since / @DeveloperApi, into spark-tags #13074

Closed

[SPARK-14613][ML] Add @Since into the matrix and vector classes in spark-mllib-local #12416

[SPARK-14613][ML] Add @Since into the matrix and vector classes in spark-mllib-local #12416

Conversation

pravingadakh commented Apr 15, 2016

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

pravingadakh Apr 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbtsai commented Apr 15, 2016

dbtsai commented Apr 15, 2016

dbtsai commented Apr 15, 2016

SparkQA commented Apr 15, 2016

holdenk commented Apr 15, 2016

srowen commented Apr 15, 2016

holdenk commented Apr 15, 2016

MLnick commented Apr 18, 2016

srowen commented Apr 18, 2016

dbtsai commented Apr 18, 2016

holdenk commented Apr 18, 2016

pravingadakh commented Apr 19, 2016

srowen commented Apr 19, 2016

rxin commented Apr 19, 2016

pravingadakh commented Apr 20, 2016

SparkQA commented Apr 20, 2016

Choose a reason for hiding this comment

pravingadakh Apr 20, 2016 • edited

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

SparkQA commented Apr 20, 2016

dbtsai commented Apr 22, 2016

pravingadakh commented Apr 23, 2016

SparkQA commented Apr 23, 2016

SparkQA commented Apr 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srowen commented Apr 23, 2016

srowen commented Apr 25, 2016

pravingadakh commented Apr 25, 2016

vanzin commented Apr 25, 2016

SparkQA commented Apr 28, 2016

SparkQA commented Apr 28, 2016

dbtsai commented Apr 28, 2016

yhuai commented Apr 29, 2016

yhuai commented Apr 29, 2016

yhuai commented Apr 29, 2016

srowen commented Apr 29, 2016

yhuai commented Apr 29, 2016

yhuai commented Apr 29, 2016

srowen commented Apr 29, 2016

vanzin commented Apr 29, 2016

pravingadakh commented Apr 29, 2016

dbtsai commented Apr 29, 2016

Choose a reason for hiding this comment

pravingadakh commented Apr 29, 2016

srowen commented Apr 30, 2016

pravingadakh commented May 2, 2016

srowen commented May 2, 2016

pravingadakh commented May 2, 2016 • edited

srowen commented May 6, 2016

srowen commented May 11, 2016

pravingadakh commented May 12, 2016

srowen commented May 12, 2016

pravingadakh Apr 15, 2016 •

edited

pravingadakh Apr 20, 2016 •

edited

pravingadakh commented May 2, 2016 •

edited