[SPARK-28855][CORE][ML][SQL][STREAMING] Remove outdated usages of Experimental, Evolving annotations #25558
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mgaido91
reviewed
Aug 22, 2019
| * @since 1.3.0 | ||
| */ | ||
| @Experimental | ||
| @Unstable |
Contributor
There was a problem hiding this comment.
does it make sense to have it Unstable? It has not changed for over 2 years...
Member
Author
There was a problem hiding this comment.
Agree, I wasn't sure about removing Unstable, but seems like some that clearly can go. Maybe anything not changed in a long time as you say.
| * Concrete implementation of a [[BaseSessionStateBuilder]]. | ||
| */ | ||
| @Experimental | ||
| @Unstable |
Contributor
There was a problem hiding this comment.
again, not sure it make sense for this to be unstable..
tgravescs
reviewed
Aug 22, 2019
| * about the resource. Please refer to [[org.apache.spark.resource.ResourceInformation]] for | ||
| * specifics. | ||
| */ | ||
| @Evolving |
Contributor
There was a problem hiding this comment.
this was just added in 3.0, it should be left.
Member
Author
There was a problem hiding this comment.
Oops right, I see it's part of GPU resources
tgravescs
reviewed
Aug 22, 2019
| * batch so that Spark can access the data row by row. Instance of it is meant to be reused during | ||
| * the entire data loading process. | ||
| */ | ||
| @Evolving |
Contributor
There was a problem hiding this comment.
with the new changes to Columnar introduced in 3.0 I wonder if we shouldn't leave this evolving.
Member
Author
There was a problem hiding this comment.
I'll revert the changes to Columnar*
tgravescs
reviewed
Aug 22, 2019
|
|
||
| /** | ||
| * :: Experimental :: | ||
| * :: :: |
Contributor
|
those changes LGTM, I only skimmed mostly and looked for ones I'm familiar with. |
|
Test build #109576 has finished for PR 25558 at commit
|
|
Test build #109587 has finished for PR 25558 at commit
|
| * @since 2.4.0 | ||
| */ | ||
| @Experimental | ||
| @deprecated("Please use 'org.apache.spark.sql.avro.functions.from_avro' instead.", "3.0.0") |
Member
There was a problem hiding this comment.
Although this is added at 2.4.0, this removal looks okay because this API is already deprecated.
Member
There was a problem hiding this comment.
+1 with removing the annotation here.
|
|
||
| import org.apache.spark.{SparkConf, SparkContext} | ||
| import org.apache.spark.annotation._ | ||
| import org.apache.spark.annotation.{DeveloperApi, Experimental, Stable, Unstable} |
Member
There was a problem hiding this comment.
So, is the only remaining instance def experimental which is natural to be Experimental?
/**
* :: Experimental ::
* A collection of methods that are considered experimental, but can be used to hook into
* the query planner for advanced functionality.
*
* @group basic
* @since 1.3.0
*/
@Experimental
@transient
@Unstable
def experimental: ExperimentalMethods = sparkSession.experimental
Member
Author
There was a problem hiding this comment.
Pretty much. This one looks inherently experimental, so, I left it.
| * Concrete implementation of a [[BaseSessionStateBuilder]]. | ||
| */ | ||
| @Experimental | ||
| @Unstable |
Member
There was a problem hiding this comment.
For SessionStateBuilder, we may want to keep @Unstable like HiveSessionStateBuilder.scala is @Unstable in this PR.
cc @gatorsmile , @cloud-fan , @hvanhovell
Member
Author
There was a problem hiding this comment.
Yeah if in doubt let's just leave it in, but, yeah I also don't know if there's doubt or what as this hasn't changed in a long while.
Member
|
Thank you so much, @srowen . This looks good to me. |
|
Test build #109589 has finished for PR 25558 at commit
|
Member
Author
|
Uh oh, this might have uncovered some latent issue with the Python Kinesis tests: I'll try to investigate separately. |
cloud-fan
reviewed
Aug 23, 2019
| * @since 1.3.0 | ||
| */ | ||
| @Experimental | ||
| @Unstable |
Contributor
There was a problem hiding this comment.
This should remain unstable, as it uses private classes Attribute Expression.
Contributor
|
I've not checked all of the changes, just one note: some interfaces are marked as unstable because they contain private classes. We should keep them unstable even if the interface itself hasn't been changed for years. |
Member
Author
|
I'll put back the |
|
Test build #109744 has finished for PR 25558 at commit
|
Member
Author
|
OK, this is always going to fail right now as it triggers Kinesis tests, and there is some existing issue with Kinesis tests in Pyspark, which I am trying to figure out at #25559 (see also #24651). This should be an entirely non-functional change, so I'd like to merge it anyway as all of the build parts pass, and even the JVM Kinesis tests pass. |
Member
|
I think most of Structured Streaming APIs in the master branch are labeled correctly. Could you revert changes of these APIs? We are waiting for DS v2 to finalize the Structured Streaming APIs, and don't want to promise a stable DS v1 APIs. |
Member
Author
|
@zsxwing would you say revert the changes to all of |
Member
Yep.
I don't have a strong option here. We unlikely change them in future. |
Member
Author
|
@zsxwing have another look. I reverted a lot of the changes to streaming. Let me know if I missed an API that should remain experimental. |
|
Test build #109820 has finished for PR 25558 at commit
|
srowen
added a commit
that referenced
this pull request
Aug 31, 2019
… that breaks Pyspark Kinesis tests
The Pyspark Kinesis tests are failing, at least in master:
```
======================================================================
ERROR: test_kinesis_stream (pyspark.streaming.tests.test_kinesis.KinesisStreamTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/jenkins/workspace/SparkPullRequestBuilder2/python/pyspark/streaming/tests/test_kinesis.py", line 44, in test_kinesis_stream
kinesisTestUtils = self.ssc._jvm.org.apache.spark.streaming.kinesis.KinesisTestUtils(2)
File "/home/jenkins/workspace/SparkPullRequestBuilder2/python/lib/py4j-0.10.8.1-src.zip/py4j/java_gateway.py", line 1554, in __call__
answer, self._gateway_client, None, self._fqn)
File "/home/jenkins/workspace/SparkPullRequestBuilder2/python/lib/py4j-0.10.8.1-src.zip/py4j/protocol.py", line 328, in get_return_value
format(target_id, ".", name), value)
Py4JJavaError: An error occurred while calling None.org.apache.spark.streaming.kinesis.KinesisTestUtils.
: java.lang.NoSuchMethodError: com.amazonaws.regions.Region.getAvailableEndpoints()Ljava/util/Collection;
at org.apache.spark.streaming.kinesis.KinesisTestUtils$.$anonfun$getRegionNameByEndpoint$1(KinesisTestUtils.scala:211)
at org.apache.spark.streaming.kinesis.KinesisTestUtils$.$anonfun$getRegionNameByEndpoint$1$adapted(KinesisTestUtils.scala:211)
at scala.collection.Iterator.find(Iterator.scala:993)
at scala.collection.Iterator.find$(Iterator.scala:990)
at scala.collection.AbstractIterator.find(Iterator.scala:1429)
at scala.collection.IterableLike.find(IterableLike.scala:81)
at scala.collection.IterableLike.find$(IterableLike.scala:80)
at scala.collection.AbstractIterable.find(Iterable.scala:56)
at org.apache.spark.streaming.kinesis.KinesisTestUtils$.getRegionNameByEndpoint(KinesisTestUtils.scala:211)
at org.apache.spark.streaming.kinesis.KinesisTestUtils.<init>(KinesisTestUtils.scala:46)
...
```
The non-Python Kinesis tests are fine though. It turns out that this is because Pyspark tests use the output of the Spark assembly, and it pulls in `hadoop-cloud`, which in turn pulls in an old AWS Java SDK.
Per Steve Loughran (below), it seems like we can just resolve this by excluding the aws-java-sdk dependency. See the attached PR for some more detail about the debugging and other options.
See #25558 (comment)
Closes #25559 from srowen/KinesisTest.
Authored-by: Sean Owen <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
srowen
added a commit
that referenced
this pull request
Aug 31, 2019
… that breaks Pyspark Kinesis tests
The Pyspark Kinesis tests are failing, at least in master:
```
======================================================================
ERROR: test_kinesis_stream (pyspark.streaming.tests.test_kinesis.KinesisStreamTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/jenkins/workspace/SparkPullRequestBuilder2/python/pyspark/streaming/tests/test_kinesis.py", line 44, in test_kinesis_stream
kinesisTestUtils = self.ssc._jvm.org.apache.spark.streaming.kinesis.KinesisTestUtils(2)
File "/home/jenkins/workspace/SparkPullRequestBuilder2/python/lib/py4j-0.10.8.1-src.zip/py4j/java_gateway.py", line 1554, in __call__
answer, self._gateway_client, None, self._fqn)
File "/home/jenkins/workspace/SparkPullRequestBuilder2/python/lib/py4j-0.10.8.1-src.zip/py4j/protocol.py", line 328, in get_return_value
format(target_id, ".", name), value)
Py4JJavaError: An error occurred while calling None.org.apache.spark.streaming.kinesis.KinesisTestUtils.
: java.lang.NoSuchMethodError: com.amazonaws.regions.Region.getAvailableEndpoints()Ljava/util/Collection;
at org.apache.spark.streaming.kinesis.KinesisTestUtils$.$anonfun$getRegionNameByEndpoint$1(KinesisTestUtils.scala:211)
at org.apache.spark.streaming.kinesis.KinesisTestUtils$.$anonfun$getRegionNameByEndpoint$1$adapted(KinesisTestUtils.scala:211)
at scala.collection.Iterator.find(Iterator.scala:993)
at scala.collection.Iterator.find$(Iterator.scala:990)
at scala.collection.AbstractIterator.find(Iterator.scala:1429)
at scala.collection.IterableLike.find(IterableLike.scala:81)
at scala.collection.IterableLike.find$(IterableLike.scala:80)
at scala.collection.AbstractIterable.find(Iterable.scala:56)
at org.apache.spark.streaming.kinesis.KinesisTestUtils$.getRegionNameByEndpoint(KinesisTestUtils.scala:211)
at org.apache.spark.streaming.kinesis.KinesisTestUtils.<init>(KinesisTestUtils.scala:46)
...
```
The non-Python Kinesis tests are fine though. It turns out that this is because Pyspark tests use the output of the Spark assembly, and it pulls in `hadoop-cloud`, which in turn pulls in an old AWS Java SDK.
Per Steve Loughran (below), it seems like we can just resolve this by excluding the aws-java-sdk dependency. See the attached PR for some more detail about the debugging and other options.
See #25558 (comment)
Closes #25559 from srowen/KinesisTest.
Authored-by: Sean Owen <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
(cherry picked from commit d5b7eed)
Signed-off-by: Sean Owen <[email protected]>
|
Test build #4850 has finished for PR 25558 at commit
|
|
Test build #4853 has finished for PR 25558 at commit
|
|
Test build #4854 has finished for PR 25558 at commit
|
Member
Author
|
Merged to master |
Member
|
Thank you so much, @srowen ! |
rluta
pushed a commit
to rluta/spark
that referenced
this pull request
Sep 17, 2019
… that breaks Pyspark Kinesis tests
The Pyspark Kinesis tests are failing, at least in master:
```
======================================================================
ERROR: test_kinesis_stream (pyspark.streaming.tests.test_kinesis.KinesisStreamTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/jenkins/workspace/SparkPullRequestBuilder2/python/pyspark/streaming/tests/test_kinesis.py", line 44, in test_kinesis_stream
kinesisTestUtils = self.ssc._jvm.org.apache.spark.streaming.kinesis.KinesisTestUtils(2)
File "/home/jenkins/workspace/SparkPullRequestBuilder2/python/lib/py4j-0.10.8.1-src.zip/py4j/java_gateway.py", line 1554, in __call__
answer, self._gateway_client, None, self._fqn)
File "/home/jenkins/workspace/SparkPullRequestBuilder2/python/lib/py4j-0.10.8.1-src.zip/py4j/protocol.py", line 328, in get_return_value
format(target_id, ".", name), value)
Py4JJavaError: An error occurred while calling None.org.apache.spark.streaming.kinesis.KinesisTestUtils.
: java.lang.NoSuchMethodError: com.amazonaws.regions.Region.getAvailableEndpoints()Ljava/util/Collection;
at org.apache.spark.streaming.kinesis.KinesisTestUtils$.$anonfun$getRegionNameByEndpoint$1(KinesisTestUtils.scala:211)
at org.apache.spark.streaming.kinesis.KinesisTestUtils$.$anonfun$getRegionNameByEndpoint$1$adapted(KinesisTestUtils.scala:211)
at scala.collection.Iterator.find(Iterator.scala:993)
at scala.collection.Iterator.find$(Iterator.scala:990)
at scala.collection.AbstractIterator.find(Iterator.scala:1429)
at scala.collection.IterableLike.find(IterableLike.scala:81)
at scala.collection.IterableLike.find$(IterableLike.scala:80)
at scala.collection.AbstractIterable.find(Iterable.scala:56)
at org.apache.spark.streaming.kinesis.KinesisTestUtils$.getRegionNameByEndpoint(KinesisTestUtils.scala:211)
at org.apache.spark.streaming.kinesis.KinesisTestUtils.<init>(KinesisTestUtils.scala:46)
...
```
The non-Python Kinesis tests are fine though. It turns out that this is because Pyspark tests use the output of the Spark assembly, and it pulls in `hadoop-cloud`, which in turn pulls in an old AWS Java SDK.
Per Steve Loughran (below), it seems like we can just resolve this by excluding the aws-java-sdk dependency. See the attached PR for some more detail about the debugging and other options.
See apache#25558 (comment)
Closes apache#25559 from srowen/KinesisTest.
Authored-by: Sean Owen <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
(cherry picked from commit d5b7eed)
Signed-off-by: Sean Owen <[email protected]>
Member
|
I went over the PR and below is the list to summarize the changes. Could you confirm whether it is complete?
|
Member
Author
|
It looks about right at a glance. I spot-checked many of them and they look correct, nothing was missing. If you did the hard work to list all of them out I take your word for it. (There weren't other pull requests besides this one) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
The Experimental and Evolving annotations are both (like Unstable) used to express that a an API may change. However there are many things in the code that have been marked that way since even Spark 1.x. Per the dev@ thread, anything introduced at or before Spark 2.3.0 is pretty much 'stable' in that it would not change without a deprecation cycle. Therefore I'd like to remove most of these annotations. And, remove the
:: Experimental ::scaladoc tag too. And likewise for Python, R.The changes below can be summarized as:
It's a big change to review, so I'd suggest scanning the list of files changed to see if any area seems like it should remain partly experimental and examine those.
Why are the changes needed?
Many of these annotations are incorrect; the APIs are de facto stable. Leaving them also makes legitimate usages of the annotations less meaningful.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests.