-
Notifications
You must be signed in to change notification settings - Fork 360
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
[SW-1421] Integrate H2O 3.26.0.1 #1348
Conversation
@@ -175,7 +175,8 @@ object H2OMOJOModel extends H2OMOJOReadable[PyH2OMOJOModel] with H2OMOJOLoader[P | |||
val is = new ByteArrayInputStream(mojoData) | |||
val reader = MojoReaderBackendFactory.createReaderBackend(is, MojoReaderBackendFactory.CachingStrategy.MEMORY) | |||
|
|||
val modelOutputJson = JsonModelDescriptorReader.parseModelJson(reader).getAsJsonObject("output") | |||
|
|||
val modelOutputJson = ModelJsonReader.parseModelJson(reader).getAsJsonObject("output") |
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.
was moved in H2O to ModelJsonReader
* @param destination directory where the logs will be downloaded | ||
*/ | ||
def downloadH2OLogs(destination: URI, logContainer: LogArchiveContainer): URI = { | ||
val workersLogs = getLogsFromWorkers(logContainer) |
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.
I created PR on H2O h2oai/h2o-3#3698 which would allow us to remove the logic for downloading logs from SW and use directly H2O implementation. Right now this is duplication code in H2O
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.
LGTM, great job!
// Close the full zip file. | ||
zos.close() | ||
val finalArchiveByteArray = try { | ||
val method = classOf[RequestServer].getDeclaredMethod("archiveLogs", classOf[LogArchiveContainer], |
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.
It would be nice to change the archiveLogs
method to public and call the method directly in a different PR.
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.
Yup -> h2oai/h2o-3#3698
try { | ||
baos.writeTo(outputStream) | ||
outputStream.write(finalArchiveByteArray) | ||
} | ||
finally { | ||
if (outputStream != null) outputStream.close() |
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.
I know that this is unrelated to this PR, but can outputStream
ever be null
?
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.
Good catch, we can remove this check!
(cherry picked from commit c795a52)
No description provided.