Move OperationInitializer Construction to Dagger Object Graph #4986
Conversation
nbauernfeind
added
configuration
core
nbauernfeind
requested review from
chipkent,
jmao-denver and
rcaudy
as code owners
| @@ -69,11 +69,18 @@ def start_jvm_for_tests(jvm_props: Dict[str, str] = None): | |||
| # Set up a Deephaven Python session | |||
| py_scope_jpy = jpy.get_type("io.deephaven.engine.util.PythonScopeJpyImpl").ofMainGlobals() | |||
| global py_dh_session | |||
|
|
|||
| no_op_thread_factory = jpy.get_type("io.deephaven.util.thread.ThreadInitializationFactory").NO_OP | |||
There was a problem hiding this comment.
I think this should follow the _ convention.
There was a problem hiding this comment.
underscore is for top-level stuff that shouldn't be imported from other modules, not necessary here
There was a problem hiding this comment.
underscore doesn't apply to auto-variables in the function scope, so the naming is fine here, though I prefer to use the _j_ or j_ prefix if an object/variable comes from Java..
| return PeriodicUpdateGraph.newBuilder(PeriodicUpdateGraph.DEFAULT_UPDATE_GRAPH_NAME) | ||
| .numUpdateThreads(PeriodicUpdateGraph.NUM_THREADS_DEFAULT_UPDATE_GRAPH) | ||
| .threadInitializationFactory(threadInitializationFactory) |
There was a problem hiding this comment.
@niloc132 Was Python debugging broken without this?
There was a problem hiding this comment.
Likely, yes, but we don't have tests for this.
| _JUpdateGraph = jpy.get_type("io.deephaven.engine.updategraph.impl.PeriodicUpdateGraph") | ||
| docs_update_graph = _JUpdateGraph.newBuilder("PYTHON_DOCS").build() | ||
| docs_update_graph = _JUpdateGraph.newBuilder("PYTHON_DOCS") \ |
There was a problem hiding this comment.
Same comment as on the test helper. Maybe consult with @jmao-denver to make sure we have the right conventions.
There was a problem hiding this comment.
these arguably could be _ prefixed... but this is configuration for running sphinx, not something that users can use (or that we would import anywhere else), so shouldn't matter.
There was a problem hiding this comment.
+1, and the same comment above applies here too.
| @@ -69,11 +69,18 @@ def start_jvm_for_tests(jvm_props: Dict[str, str] = None): | |||
| # Set up a Deephaven Python session | |||
| py_scope_jpy = jpy.get_type("io.deephaven.engine.util.PythonScopeJpyImpl").ofMainGlobals() | |||
| global py_dh_session | |||
|
|
|||
| no_op_thread_factory = jpy.get_type("io.deephaven.util.thread.ThreadInitializationFactory").NO_OP | |||
There was a problem hiding this comment.
underscore doesn't apply to auto-variables in the function scope, so the naming is fine here, though I prefer to use the _j_ or j_ prefix if an object/variable comes from Java..
| _JUpdateGraph = jpy.get_type("io.deephaven.engine.updategraph.impl.PeriodicUpdateGraph") | ||
| docs_update_graph = _JUpdateGraph.newBuilder("PYTHON_DOCS").build() | ||
| docs_update_graph = _JUpdateGraph.newBuilder("PYTHON_DOCS") \ |
There was a problem hiding this comment.
+1, and the same comment above applies here too.
|
Nightlies passed: https://github.com/nbauernfeind/deephaven-core/actions/runs/7455524151 |
This sets the proper operation initializer for the refresh thread should the UpdateGraph be started in single threaded mode. This was detected in #4975 and is a part of that fix.