Skip to content
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

Remote Debugger #8

Merged
merged 9 commits into from Jun 9, 2022
Merged

Remote Debugger #8

merged 9 commits into from Jun 9, 2022

Conversation

carlsverre
Copy link
Contributor

@carlsverre carlsverre commented Jun 7, 2022

This diff implements a remote debugger which can help test wasm udfs being called by SingleStore.

The goals are:

  • support running in lldb + breakpoints
  • use external functions in singlestore
  • be able to log to the console for print() style debugging
  • minimum amount of additional overhead to enable debugging

I tried building this in a bunch of different ways - but ended up deciding to have the debugger depend on the wasm module exporting a specific entrypoint. This is currently simpler than dynamically generating hander code for each of the example projects.

With this model, the debugger binary will listen on an http port for requests from SingleStore (external functions). Since external functions heavily batches requests, the debugger will unpack the rows list at the top level and then invoke the wasm module for each row. The input to the wasm module will be a "name" derived from the endpoint along with the current row encoded as a json array (basically passthrough from SingleStore). The wasm module will have to export the debugger's handle_json method, process the incoming row, call it's internal behavior, and then return a json array as the result.

To simplify usage a proc macro is included. See the debugger readme for examples and documentation.

For now, you will need to manually create the external function in SingleStore. Also, as external functions only support scalar-inputs, if you want to pass records in you will need to wrap them in to_json. Something like this should work:

select * from t(to_json(row("hi"):>record(t text)));

@carlsverre
Copy link
Contributor Author

carlsverre commented Jun 7, 2022

If you want to test this - use vscode. Then open up examples/rust/power/src/lib.rs, put a break point on line 18, and hit f5 to debug.

@carlsverre carlsverre marked this pull request as ready for review June 9, 2022 18:17
@carlsverre
Copy link
Contributor Author

Writing the readme now.

@@ -0,0 +1,84 @@
use proc_macro::TokenStream;
use proc_macro2::TokenStream as TokenStream2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we need both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a long story - short story: proc_macro2 has a bunch of nice extensions which make building proc macros much easier. Mostly related to interpolating in/out of other streams via the quote! macro. Basically, we want to keep everything in TokenStream2 until we finally return it from the proc_macro which has to use TokenStream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope someone just merges the proc_macro2 crate into upstream at some hypothetical future point in time since it's pretty silly. (there are lots of reasons that hasn't happened and may not ever happen... fun rust compiler stuff)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙃

@esoterra
Copy link
Collaborator

esoterra commented Jun 9, 2022

Please update your .wit files to use func instead of function


wit_bindgen_rust::export!("sentiment.wit");

impl Serialize for sentiment::PolarityScores {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shame that there isn't a way to add derives to WIT types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omg I agree 100%. I tried to hack this in a bunch of different ways. Even considered doing it in wit-bindgen, but it would require some annotation style idea in wit that may not be long-term desirable. Actually, what would likely be the most general solution is to have wit-bindgen support attaching onto previously defined structs rather than needing to generate all the types themselves. That might be a bit tricky, but would allow the user to just define the struct themself along with any additional derives needed.

@esoterra esoterra merged commit 643a567 into main Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants