-
Notifications
You must be signed in to change notification settings - Fork 80
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
Rewire python<->java logging to be generally friendlier to python #2763
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
243538b
Rewire python<->java logging to be generally friendlier to python
niloc132 f55fcb7
Review feedback
niloc132 f6ba93d
Attempt to apply correct doc style
niloc132 5c92be0
review feedback
niloc132 afa01e0
Add TODOs for followup
niloc132 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
49 changes: 49 additions & 0 deletions
49
Integrations/src/main/java/io/deephaven/integrations/python/PyLogOutputStream.java
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package io.deephaven.integrations.python; | ||
|
||
import org.jetbrains.annotations.NotNull; | ||
import org.jpy.PyObject; | ||
|
||
import java.io.IOException; | ||
import java.io.OutputStream; | ||
import java.util.function.Supplier; | ||
|
||
/** | ||
* Simple output stream that redirects all writes to a python io.TextIOBase type. | ||
*/ | ||
public class PyLogOutputStream extends OutputStream { | ||
private final Supplier<PyObject> rawIoBaseSupplier; | ||
|
||
public PyLogOutputStream(Supplier<PyObject> rawIoBaseSupplier) { | ||
this.rawIoBaseSupplier = rawIoBaseSupplier; | ||
} | ||
|
||
@Override | ||
public void write(int i) throws IOException { | ||
write(new byte[] {(byte) i}); | ||
} | ||
|
||
@Override | ||
public void write(@NotNull byte[] b) throws IOException { | ||
// TODO (deephaven#2793) switch to modern method overloads when jpy#87 is fixed | ||
rawIoBaseSupplier.get().callMethod("write", new String(b)); | ||
} | ||
|
||
@Override | ||
public void write(@NotNull byte[] b, int off, int len) throws IOException { | ||
byte[] buffer = new byte[len]; | ||
System.arraycopy(b, off, buffer, 0, len); | ||
write(buffer); | ||
} | ||
|
||
@Override | ||
public void flush() throws IOException { | ||
// TODO (deephaven#2793) switch to modern method overloads when jpy#87 is fixed | ||
rawIoBaseSupplier.get().callMethod("flush"); | ||
} | ||
|
||
@Override | ||
public void close() throws IOException { | ||
// TODO (deephaven#2793) switch to modern method overloads when jpy#87 is fixed | ||
rawIoBaseSupplier.get().callMethod("close"); | ||
} | ||
} |
This file contains 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
66 changes: 0 additions & 66 deletions
66
Integrations/src/main/java/io/deephaven/integrations/python/PythonLogAdapter.java
This file was deleted.
Oops, something went wrong.
This file contains 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
This file contains 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
49 changes: 49 additions & 0 deletions
49
...ded-server/java-runtime/src/main/java/io/deephaven/python/server/EmbeddedPyLogModule.java
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package io.deephaven.python.server; | ||
|
||
import dagger.Module; | ||
import dagger.Provides; | ||
import dagger.multibindings.ElementsIntoSet; | ||
import io.deephaven.base.system.StandardStreamState; | ||
import io.deephaven.internal.log.InitSink; | ||
import io.deephaven.internal.log.LoggerFactory; | ||
import io.deephaven.io.log.LogSink; | ||
import io.deephaven.io.logger.LogBuffer; | ||
import io.deephaven.io.logger.LogBufferGlobal; | ||
import io.deephaven.server.log.LogModule; | ||
|
||
import javax.inject.Singleton; | ||
import java.util.Collections; | ||
import java.util.ServiceLoader; | ||
import java.util.Set; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.StreamSupport; | ||
|
||
@Module | ||
public interface EmbeddedPyLogModule { | ||
|
||
@Provides | ||
static LogBuffer providesLogBuffer() { | ||
return LogBufferGlobal.getInstance().orElseThrow(() -> new RuntimeException("No global LogBuffer found")); | ||
} | ||
|
||
@Provides | ||
static LogSink providesLogSink() { | ||
// In contract, this should be a singleton - see LogInit#checkLogSinkIsSingleton() | ||
return LoggerFactory.getLogger(LogModule.class).getSink(); | ||
} | ||
|
||
@Provides | ||
@ElementsIntoSet | ||
static Set<InitSink> providesLoggerSinkSetups() { | ||
return StreamSupport | ||
.stream(ServiceLoader.load(InitSink.class).spliterator(), false) | ||
.collect(Collectors.toSet()); | ||
} | ||
|
||
@Provides | ||
@Singleton | ||
static StandardStreamState providesStandardStreamState() { | ||
return new StandardStreamState(Collections.emptySet()); | ||
} | ||
|
||
} |
This file contains 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
Oops, something went wrong.
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.
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.
Do we need / want to account for Charset here?
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.
IMO, it would be better if we could call
org.jpy.PyObject#call(java.lang.Class<T>, java.lang.String, java.lang.Class<?>[], java.lang.Object[])
- this allows us to be more explicit w/ the parameter and return types.As it stands now, all of the calls to
callMethod
returnPyObject
, which I think is extra garbage we shouldn't need to create.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.
Yeah, that raises a question - why can jpy convert cleanly from py bytes to java byte[], but not vice versa? That's the real answer here, since it is already encoded (and we can't cleanly ask for the underlying encoding... since jpy might break it).
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.
That said, why be more explicit with the param types? Both call into
PyLib.callAndReturnValue
, but the variant that takes lots ofClass<?>
s does java-side type checks, rather than just passing the string instance - and there is no return type to be concerned with.(Also,
.call()
vs.callMethod()
has semantics that technically we don't want here, we actually wantcallMethod
. However, as far as I can tell, jpy just throws away that extra flag with a "todo why isn't this used?" sort of note)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'm mainly concerned about creating extraneous PyObjects - "write" will return a PyObject that is a python int.
vs
IMO, the second is much better.
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.
To be clear, these are not the same -
call
sets methodCall=false and its docs clarify that this is invoking a python callable object, not a method. All overrides do this.In contrast,
callMethod
sets methodCall=true, and is intended for calling methods. We don't have (but arguably should?) an overload ofcallMethod
likecall
does to support each arg type. I'd support going so far as to add overloads that return primitives or void, to avoid that extra cost you describe.In this case, we want to call a method, we are not interacting with a callable object.
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 jpy implementation does not actually use that boolean internally - some sort of historic wart before my time.
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.
Yes - that's what I said above - in which case we should either figure out what issue might be there or historically have been there, or change the API. I'm just going off of the current stated contract of JPy, which is that this isn't to be used for method calls. But at the end of the day, we're using the wrong method that happens to do the right thing, for now, probably, to save on creating one object (that will either be called approx once per jvm, in the case of close(), or for write and maybe flush, are likely to be jitted to be inlined and have escap analysis handle anyway)?
I've you insist, I'll write the wrong code that currently does what we want but not what we're asking for, but this is an improvement to make to jpy and then improve our usage of it, so I'm objecting to deliberately doing the wrong thing.
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've filed jpy-consortium/jpy#87.
org.jpy.PyObject#call(java.lang.Class<T>, java.lang.String, java.lang.Class<A0>, A0)
doesn't expose the notion of "methodCall", or have any javadoc (my fault). I think your argument falls down a little bit though, b/c if the implementation of that method instead usedtrue
instead offalse
, the outcome would be exactly the same, and is arguably an implementation detail.I do like to consider myself against unnecessary pre-optimization. With respect to PyObject though, it's not the allocation itself I'm usually worried about, but the implications that extraneous PyObjects may cause (PyObjectReferences growth, unnecessarily keeping python references alive). And specifically as a glue layer between streams, there's potential for this to be heavily used. In general, if there is a way to accomplish the same outcome but without creating a PyObject, that's the direction I'll lean.
All that said - I've argued my case, and can let the code stand as-is if that's still your preference.