Added language detection #42

mcarmonaa · 2017-09-19T07:31:21Z

Currently is needed to have enry-java in your local ivi2 repo.

bzz · 2017-09-19T08:08:43Z

Awesome 🎉 I supposes, this addresses #6
Let me try it out locally and get back to you later today.

CI fails on missing enry-java dependency.

sbt.ResolveException: unresolved dependency: tech.sourced#enry-java;1.0: not found

We can not use jitpack for enry-java as the default build would most probably not be a fatjar one, with all the native dependencies. Until src-d/enry#102 is resolved, you could copay and add an existing helper script ./local-install-enry-java.sh to .travis.yml as a workaround.

@mcarmonaa in the meanwhile, could you please double-check that the exposed API is the same as in the design document?

bzz · 2017-09-19T08:17:58Z

src/test/scala/tech/sourced/api/BaseSparkSpec.scala

@@ -14,6 +15,12 @@ trait BaseSparkSpec extends BeforeAndAfterAll {
      .appName("test").master("local[*]")
      .config("spark.driver.host", "localhost")
      .getOrCreate()
+
+    Logger.getLogger("org").setLevel(Level.OFF)
+    Logger.getLogger("akka").setLevel(Level.OFF)


Do you think it's still useful? Right now, logging for tests is muted and configured though log4j.properties

Indeed it doesn't seem so much useful now, I'll remove these lines.

bzz · 2017-09-19T08:20:55Z

src/test/scala/tech/sourced/api/BaseSparkSpec.scala

+    Logger.getLogger("akka").setLevel(Level.OFF)
+
+    import Implicits._
+    ss.registerUDFs()


Not sure, but is there a reason we want to expose this to clients?
Are there any downsides in this, being always done by Implicits, so user does not need to do that ?

Since the spark-session is an object outside our library, and Implicits allow us augment functionality of some classes performing conversions from those classes to our own defined classes, there's no way to register automatically the udfs just importing Implicits. So is needed to do it by hand before use them.

erizocosmico · 2017-09-19T08:24:21Z

Maybe we can get enry via jitpack for now. It "should" work after this, I think src-d/enry#98

bzz · 2017-09-19T08:26:28Z

👍 @erizocosmico I guess it also depends, if jitpack uses sbt assembly or not. Definitely worth a try!

erizocosmico · 2017-09-19T08:28:33Z

@bzz ah, right, then probably it doesn't work. We might need to look into making the assembly build with the normal packaging task

mcarmonaa · 2017-09-19T09:19:01Z

I performed changes to follow the design document, so now the exported method is classifyLanguages, and I removed the logger lines too.

bzz · 2017-09-19T09:49:24Z

src/main/scala/tech/sourced/api/Implicits.scala

+
+    def classifyLanguages: DataFrame = {
+      Implicits.checkCols(df, "file_hash")
+      df.withColumn("lang", ClassifyLanguagesUDF.function(col("is_binary"), col("path"), col("content")))


Is there a reason why file_hash column is checked to be present in df, rather then is_binary, path and content that are actually used below?

Oops!, fixing in progress

bzz · 2017-09-19T09:54:43Z

src/main/scala/tech/sourced/api/customudf/CustomUDF.scala

+
+  private def getLanguage(isBinary: Boolean, path: String, content: Array[Byte]): Option[String] = isBinary match {
+    case false => {
+      val lang = Enry.getLanguage(path, content)


Probably a nitpick, but do you think

... = isBinay match { case true => // ... case false => None }

has advantages over

... = if (isBinary) { // ... } else { None }

?

No I don't, I just wanted to use new scala patterns I learnt, but probably is more readable use an if expression here. I'll change it

bzz · 2017-09-19T09:56:00Z

@mcarmonaa nice job! Few more comments above.

Sorry, must have missed where another comment was addressed - do you see any reason not to make CI pass with .sh script?

bzz · 2017-09-19T10:06:39Z

@mcarmonaa Regarding now hidden discussion

ss.registerUDFs()

Do you think this approach f31b1e5 would not work for some reason?

I think it may be reasonable to have a convention of UDFs registering themselves, thus removing this "cognitive burden" from the API users.

mcarmonaa · 2017-09-19T10:12:40Z

@bzz about f31b1e5, Is it ok register the udf in the catalog each time you use it?

codecov · 2017-09-19T11:19:44Z

Codecov Report

Merging #42 into master will decrease coverage by <.01%.
The diff coverage is 87.5%.

@@             Coverage Diff              @@
##             master      #42      +/-   ##
============================================
- Coverage     83.42%   83.42%   -0.01%     
- Complexity       31       32       +1     
============================================
  Files            13       14       +1     
  Lines           362      374      +12     
  Branches         62       65       +3     
============================================
+ Hits            302      312      +10     
  Misses           27       27              
- Partials         33       35       +2

Impacted Files	Coverage Δ	Complexity Δ
...n/scala/tech/sourced/api/customudf/CustomUDF.scala	`85.71% <85.71%> (ø)`	`1 <1> (?)`
src/main/scala/tech/sourced/api/Implicits.scala	`81.25% <88.88%> (-0.24%)`	`0 <0> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c4d85f...adc9e15. Read the comment docs.

mcarmonaa · 2017-09-19T11:24:53Z

As talking irl with @bzz, by the moment

ss.registerUDFs()

still will be there.

The rest of the feedback has been addressed

bzz · 2017-09-19T11:36:24Z

@mcarmonaa awesome work! ~~Let's wait for CI to become green~~ CI is green now, LGTM to merge and I'll be happy to rebase bblfsh part in #41 to adhere the same conventions for UDFs.

bzz · 2017-09-19T11:47:07Z

and 👍 for having nice tests!

erizocosmico

Other than these two minor things, LGTM, great job! 🎉

erizocosmico · 2017-09-19T13:16:06Z

build.sbt


    resolvers += "jitpack" at "https://jitpack.io",
+    resolvers += "Local Ivy repository" at "file://"+Path.userHome.absolutePath+"/.ivy2/repository",


I would add a TODO or FIXME to explain why this is here, just in case we forget to delete it after enry is published

erizocosmico · 2017-09-19T13:20:16Z

src/main/scala/tech/sourced/api/customudf/CustomUDF.scala

+
+
+
+


Yes it is, I'll remove it

mcarmonaa requested review from ajnavarro and bzz September 19, 2017 07:31

bzz suggested changes Sep 19, 2017

View reviewed changes

bzz reviewed Sep 19, 2017

View reviewed changes

bzz suggested changes Sep 19, 2017

View reviewed changes

bzz approved these changes Sep 19, 2017

View reviewed changes

bzz mentioned this pull request Sep 19, 2017

python: SparkAPI python wrapper #39

Merged

5 tasks

bzz requested a review from erizocosmico September 19, 2017 12:11

erizocosmico approved these changes Sep 19, 2017

View reviewed changes

mcarmonaa added 4 commits September 19, 2017 16:28

added enry dependency

8b482b9

added enry udf

017fd3e

added tests for enry udf

60f2e71

added script to allow travis get java-enry

adc9e15

bzz merged commit b2ca969 into src-d:master Sep 19, 2017

mcarmonaa deleted the feature/enry-languages branch September 20, 2017 10:44

bzz mentioned this pull request Sep 21, 2017

[DOC] Installations instructions #34

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added language detection #42

Added language detection #42

mcarmonaa commented Sep 19, 2017

bzz commented Sep 19, 2017 •

edited

Loading

bzz Sep 19, 2017

mcarmonaa Sep 19, 2017

bzz Sep 19, 2017

mcarmonaa Sep 19, 2017

erizocosmico commented Sep 19, 2017

bzz commented Sep 19, 2017

erizocosmico commented Sep 19, 2017

mcarmonaa commented Sep 19, 2017

bzz Sep 19, 2017

mcarmonaa Sep 19, 2017

bzz Sep 19, 2017

mcarmonaa Sep 19, 2017

bzz commented Sep 19, 2017 •

edited

Loading

bzz commented Sep 19, 2017 •

edited

Loading

mcarmonaa commented Sep 19, 2017

codecov bot commented Sep 19, 2017 •

edited

Loading

mcarmonaa commented Sep 19, 2017 •

edited

Loading

bzz commented Sep 19, 2017 •

edited

Loading

bzz commented Sep 19, 2017

erizocosmico left a comment

erizocosmico Sep 19, 2017

erizocosmico Sep 19, 2017

mcarmonaa Sep 19, 2017


		resolvers += "jitpack" at "https://jitpack.io",
		resolvers += "Local Ivy repository" at "file://"+Path.userHome.absolutePath+"/.ivy2/repository",

Added language detection #42

Added language detection #42

Conversation

mcarmonaa commented Sep 19, 2017

bzz commented Sep 19, 2017 • edited Loading

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

erizocosmico commented Sep 19, 2017

bzz commented Sep 19, 2017

erizocosmico commented Sep 19, 2017

mcarmonaa commented Sep 19, 2017

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

bzz commented Sep 19, 2017 • edited Loading

bzz commented Sep 19, 2017 • edited Loading

mcarmonaa commented Sep 19, 2017

codecov bot commented Sep 19, 2017 • edited Loading

Codecov Report

mcarmonaa commented Sep 19, 2017 • edited Loading

bzz commented Sep 19, 2017 • edited Loading

bzz commented Sep 19, 2017

erizocosmico left a comment

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

bzz commented Sep 19, 2017 •

edited

Loading

bzz commented Sep 19, 2017 •

edited

Loading

bzz commented Sep 19, 2017 •

edited

Loading

codecov bot commented Sep 19, 2017 •

edited

Loading

mcarmonaa commented Sep 19, 2017 •

edited

Loading

bzz commented Sep 19, 2017 •

edited

Loading