-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Connect to DHE worker #134
Conversation
d6f7b3a
to
bbd0962
Compare
8c35183
to
8ebb11d
Compare
45de264
to
13fd66f
Compare
End-to-end Test Summary
Detailed Test Results
Failed Test SummaryNo failed tests ✨Flaky Test SummaryNo flaky tests detected. ✨ |
08b8b6b
to
0c51803
Compare
16f9bef
to
c953d79
Compare
src/dh/dhe.ts
Outdated
* @param matchSerial Serial to match. | ||
* @returns The matching `QueryInfo` or `undefined` if not found. | ||
*/ | ||
export function findQueryConfigForSerial( |
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 don't know if it matters in your case, but known configs may be not fully populated on login. The client fires configadded
for each of the remaining queries.
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.
getWorkerInfoFromQuery
calls this function in response to EVENT_CONFIG_UPDATED
events until it finds a match. Should that work, or does EVENT_CONFIG_ADDED
need to be accounted for as well?
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.
Scanning all known configs on each update can be problematic with something like 10k+ queries since the update event is fired multiple times in the query lifecycle.
I think EVENT_CONFIG_ADDED
needs to be handled instead of EVENT_CONFIG_UPDATED
. Also check if event.details have the info you need so you don't have to re-scan the known configs.
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.
IIR, I had originally tried that one, but the worker url wasn't available when it was added. It only showed up after a future update. I could be misremembering though.
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.
Right, that seems possible. Was the worker URL eventually available in the update event details?
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
src/dh/dhe.ts
Outdated
serverConfigValues.timeZone ?? | ||
Intl.DateTimeFormat().resolvedOptions().timeZone; | ||
|
||
const scheduling = QueryScheduler.makeDefaultScheduling( |
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 seems possible to create an IC query with an arbitrary scheduling type, though Charles mentioned it should be Temporary scheduling with the InteractiveConsoleTemporaryQueue
queue name. See make_temporary_config
for more details - https://docs.deephaven.io/pycoreplus/20240517/_modules/deephaven_enterprise/client/controller.html#ControllerClient.make_temporary_config
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.
@vbabich
Looks like that calls:https://docs.deephaven.io/pycoreplus/20240517/_modules/deephaven_enterprise/client/generate_scheduling.html#GenerateScheduling.generate_temporary_scheduler
Does that mean instead of calling makeDefaultScheduling
, we'd want to add a new method like makeTemporaryScheduling
and return an array of key=value tokens similar to generate_temporary_scheduler
?
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 created a PR based on the referenced code that "should" give us a comparable temporary schedule in the JS client:
https://github.com/deephaven-ent/iris/pull/2348
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, makeDefaultScheduling
generates the scheduling config for new Script type queries I'd add a function for temporary scheduling.
src/dh/dhe.ts
Outdated
// This Promise will respond to config update events and resolve when the worker | ||
// is ready. | ||
const queryInfo = await new Promise<QueryInfo>(resolve => { | ||
dheClient.addEventListener(dhe.Client.EVENT_CONFIG_UPDATED, _event => { |
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.
EVENT_CONFIG_UPDATED
is fired fairly often and scanning known configs can be problematic on large query sets. I'd do an initial findQueryConfigForSerial
and subscribe to the update event if the info object with the URL isn't found.
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.
A couple of notes:
addEventListener
returns a callback remover. Need to remove the listener on resolve/reject.- the Promise gets stuck in Pending if the query is created but ends up in a Failed state. Might need to check if the query status is one of the valid startup statuses.
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.
…m the right type. (#79)
Initial DHE connection feature:
Testing
Setup
Connect to VPN, and have an available DHE server (dev-sanluis is what I've been using)
Configure the DHE server in vscode user or workspace settings:
Pull this branch, npm install packages, then f5 for debugging. It should automatically open another vscode instance. In that instance, open a vscode folder containing .py / .groovy scripts, or create a new one where files can be put (vscode should remember this folder the next time you run the debugger)
Click the DH activity bar item. You should see your configured server in the list of "Running" servers:
Click on the server to initiate login flow. Provide credentials
A worker should be created (should see it loading in the CONNECTIONS panel
Create a new python file with a simple script:
Run it using double arrows at top of file.
If you don't have any other configured, running server, it should automatically connect to the worker you started. Otherwise, you will be prompted to select your connection
To discard the worker, you cal use the trash icon in the connection tree item