-
Notifications
You must be signed in to change notification settings - Fork 392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change spark-tensorflow-connector dependency to be spark 3.0.0 preview #141
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@@ -86,7 +87,7 @@ | |||
<configuration> | |||
<recompileMode>incremental</recompileMode> | |||
<useZincServer>true</useZincServer> | |||
<scalaVersion>${scala.binary.version}</scalaVersion> | |||
<scalaVersion>${scala.version}</scalaVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need specify scala version "2.12.10" instead of "2.12" here, otherwise it will cause some compatibility issue inside maven scala plugin.
...flow-connector/src/main/scala/org/tensorflow/spark/datasources/tfrecords/DefaultSource.scala
Show resolved
Hide resolved
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@jhseu Could you help review it ? Thanks! |
When I try to build this, I'm hitting:
It looks like this tries to get a tensorflow-hadoop version which matches the spark-tensorflow-connector version. Is that intentional (given that tensorflow-hadoop is on version 1.14.0, whereas spark-tensorflow-connector is on version 1.10.0)? |
@jkbradley The default maven repo only include tensorflow-hadoop version >= 1.11,
|
Whoops, my bad, did not realize it's in the same project & is a manually handled dependency. Thanks! |
Since this project's CI isn't running, I tested this PR locally. It may have some flakiness in the impl or tests right now. I ran the tests once (mvn clean install) and hit the following failure. But then I ran them again (mvn test) & they passed. I ran a 3rd time (mvn clean install) and they passed. Failure in LocalWriteSuite:
Also, one nit: the name of the artifact in the pom should be updated to 2.12: spark-tensorflow-connector_2.11 |
I'm not opposed to this, but wouldn't it be better to wait until Spark 3.0.0 is released? |
@jhseu After we verify correctness, we can keep this PR open so less work for users who want to try out Spark 3.0 preview with spark-tensorflow-connector. |
Yeah, I don't mind keeping this open. |
@jkbradley Flaky test fixed. You could retest it. And pom artifact is updated to 2_12. |
@WeichenXu123 Could you explain the test flakiness? Is it relevant to Spark 3.0 upgrade? If not, let's submit another PR so the fix can go in. |
This reverts commit 0a6c412.
@jhseu If we do not plan to make a new release that is 2.4 compatible, shall we review and merge this PR? |
Hi, we would like to use this library with spark 2.4 and scala 2.12.10. Would it be possible to support multiple versions with multiple profiles? I should probably create an issue but just wanted to ask here as well. |
Now Spark 3.0.0 is released. And we need https://mvnrepository.com/artifact/org.tensorflow/spark-tensorflow-connector_2.12 to be released, I think. |
Change spark-tensorflow-connector to be spark-3.0.0-preview2
Test: