-
Notifications
You must be signed in to change notification settings - Fork 828
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
fix: Unable to run saveNativeModel for VWRegressionModel #1364 #1366
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -228,7 +228,7 @@ class BinaryOutputWriter(val path: String, | |||
override def write(row: InternalRow): Unit = { | |||
val bytes = row.getBinary(bytesCol) | |||
val filename = row.getString(pathCol) | |||
val nonTempPath = new Path(path).getParent.getParent.getParent.getParent.getParent |
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.
The bunch of .getParent
calls are unnecessary. Spark's output committer will move the files to the main output directory. Having these calls doesn't work in cloud environment.
@annotation.tailrec | ||
private def catchFlakiness[T](times: Int)(f: => Option[T]): Option[T] = { | ||
try { | ||
Try { | ||
f | ||
} catch { | ||
case e: NullPointerException if times >= 1 => | ||
logWarning("caught null pointer exception due to jvm bug", e) | ||
} match { | ||
case Success(x) => x | ||
case Failure(exception: NullPointerException) if times >= 1 => | ||
logWarning("caught null pointer exception due to jvm bug", exception) | ||
catchFlakiness(times - 1)(f) | ||
case _: Exception => | ||
None | ||
case _ => None | ||
} | ||
} |
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.
Not related to the bug fix, but changed the function into functional style and add tailrec annotation.
Codecov Report
@@ Coverage Diff @@
## master #1366 +/- ##
==========================================
+ Coverage 84.77% 84.85% +0.07%
==========================================
Files 287 287
Lines 14230 14230
Branches 732 731 -1
==========================================
+ Hits 12064 12075 +11
+ Misses 2166 2155 -11
Continue to review full report at Codecov.
|
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.
VW changes look good to me. Assume this ran through a unit test?
yes. The function is covered by unit test and gets run in CI build. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@eisber Markus, the merge is blocked because it requires Mark's approval as well and he's out of office. Can you force merge? |
unfortunately I cannot. maybe @serena-ruan can help? |
No description provided.