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

Wasm rework #2825

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Wasm rework #2825

wants to merge 6 commits into from

Conversation

bakaq
Copy link
Contributor

@bakaq bakaq commented Feb 14, 2025

This reworks the Wasm API to match the Rust API and therefore be much more flexible (query iterators allowing continuous feedback and infinite leaf answers, actual term data representation instead of just strings, etc...). This was some of the trickiest Rust code I've ever wrote (self-referential structs, single threaded channels, etc...), but I managed to do all of it without using unsafe. If anyone has suggestions on how to do it better please tell me. If anyone has suggestions on how to make the interface more idiomatic for Javascript I would also like to hear it!

This is still very rough and needs polish (and documentation, which I intentionally deferred), but already works really well as is.

Non-goals for this PR:

  • Have a way to capture stdout like the removed eval_code, and some way to interface with stdin. This is needed for eventually running the CLI toplevel in Scryer Playground. Doing this properly depends on Callback streams for use as library #2799, and I will do that in another PR. Just having access to queries is already powerful enough for the vast majority of use cases.
  • Settle on a serialization. This would definitely have been easier to do if we had decided on a serialization (Term and LeafAnswer serde serialization #2707), but it doesn't depend on it.
  • WASI or pure Wasm. These are also applications of Wasm I think we should tackle eventually, but this PR is concerned specifically with embedding Scryer Prolog in Javascript (the browser and things like NodeJS). WASI doesn't need any special treatment interface-wise, and pure Wasm will come for free with ISSUE-2464: Make scryer prolog usable as a shared library for other programs #2464.

@bakaq bakaq marked this pull request as draft February 14, 2025 10:12
@jjtolton
Copy link

Working great!

Added support in libscryer-clj.

For sake of completion, will mention only thing I've noticed missing so far is consult capability.

Working fantastic!

It's a low level test, will clean up the API, but couldn't be more psyched that it's working in the browser with ITERATIVE QUERIES!!! WOOOO!!!

image

@guregu
Copy link

guregu commented Feb 16, 2025

Playing around with this a bit, got an npm package (scryer) that embeds the wasm binary published.
Now you can import it directly from sites like esm.sh, like this: https://jsfiddle.net/a8hcuf6y/2/
Repo here: https://github.com/guregu/scryer-js

Some thoughts/nitpicks/observations follow (caveat: I'm not super familiar with wasm-bindgen/wasm-pack so I might be missing some things)

It seems like it's not possible to import the term types (PrologAtom etc.) because scryer_prolog.js doesn't re-export them (using this to build it: wasm-pack build --target web -- --no-default-features).

The classes should declare their members like so:

    export class PrologInteger {
    	type = "integer"; // here
     	integer;          // and here
        constructor(integerStr) {
            this.integer = BigInt(integerStr);
        }
    }

Nitpick: new MachineBuilder() would be more idiomatic than MachineBuilder.new(). This kind of thing I'm totally happy to hide behind a wrapper, I understand it's kind of a Rustism directly exported.

Starting a query with a syntax error (example: forgetting the dot) makes the Rust code panic with "unreachable".

Anyway, so good so far! I'll keep hacking on this package as the wasm interface coalesces, and I think it won't be too long before we can get parity with trealla-js. I'd be happy to donate the code/npm package name/etc later on once the dust settles.

@jjtolton
Copy link

Great work with scryer-js, @guregu ! Way more ergonomic to install via yarn/npm than build via rust. This will make Scryer much more accessible! 🥳

@bakaq
Copy link
Contributor Author

bakaq commented Feb 16, 2025

Some thoughts/nitpicks/observations follow (caveat: I'm not super familiar with wasm-bindgen/wasm-pack so I might be missing some things)

And I'm not very familiar with Javascript so these nitpicks are exactly what I'm looking for!

It seems like it's not possible to import the term types (PrologAtom etc.) because scryer_prolog.js doesn't re-export them (using this to build it: wasm-pack build --target web -- --no-default-features).

This was intentional, but also temporary. I wanted to just have it work first, and doing this for all term types is a lot of boilerplate. Also, I currently use the inline_js feature of wasm-bindgen to implement all these types, but that limits the export to ES Modules (so no support for --target no-modules or NodeJS if I understand the documentation correctly). Doing this right so it's portable to any Javascript runtime will mean doing all of it manually in Rust instead, which I plan to do soon. Also, do you have any suggestions on how to expose terms? I've created a different class for each variant because there are no tagged unions in Javascript, but I don't know if this is the best way to do it. I also put Prolog before the names to avoid name conflicts, but I'm not sure if this is necessary. I used Tau Prolog as a reference, but their callback-based interface is so different from what we have here with Scryer that I wasn't able to get much out of it. How do you do it in Trealla?

Nitpick: new MachineBuilder() would be more idiomatic than MachineBuilder.new(). This kind of thing I'm totally happy to hide behind a wrapper, I understand it's kind of a Rustism directly exported.

Sure! Also, is the builder pattern common in Javascript or is there a better way that this may be exposed? The builder pattern is used in Rust right now to configure the Machine's streams and toplevel, and it can be easily extended to add more configuration options without breaking changes in the future.

Starting a query with a syntax error (example: forgetting the dot) makes the Rust code panic with "unreachable".

This is actually a problem in the Rust interface! I plan to deal with it there eventually, and so it will automatically be fixed here also.

@guregu
Copy link

guregu commented Feb 17, 2025

Here's how I did my Term types, it's also kind of inspired by Tau in that they are all classes and discriminated by instanceof. Not totally sure this is the best approach. https://github.com/guregu/trealla-js/blob/9c9cc972c795895fde93bf02783cb73935e3bcdc/src/term.ts#L3-L137. The nice thing about classes is you can give them all a toProlog() or similar method that gives you an escaped string representation of the term, makes them a bit easier to work with. For parsing I give a 'reviver' argument to JSON.parse that finds terms and constructs them.
The alternative would be plain objects that use the "type" property to discriminate. It could look something like this in Typescript: https://github.com/guregu/scryer-js/blob/1c7338b1edb6f92c917b0e904005993c84b5af02/src/scryer.ts#L26-L77. These are easy to construct and parse, just a plain JSON.parse will do the trick, but would require a static toProlog() that works on all of them (which I ended up adding to my stuff anyway, so it's not really a tradeoff, haha). This is probably the closest thing to a tagged union, and Typescript does a good job handling them.
As for names, I think it's OK to drop "Prolog" from most them, since they don't have conflicts. I think String might be the only one that does have a conflict with the String() constructor so that might need a prefix or a rename (Chars?), unless we decide to go with plain strings instead of a class. It's pretty easy to rename imports so not a huge issue either way.

Speaking of imports, personally I think it's fine to stick with ESModules (also called 'esnext' build target). These days I write all my JS stuff using them. Bundlers can convert them into the other module systems if necessary. There's some nice new tooling around modules too (such as esm.sh, and the new npm alternative https://jsr.io/). I like how easy it is to import modules in browsers and start hacking with them.

Re: builder pattern. I think it's fine, there are other JS projects like ORMs (Prisma comes to mind?) that use a similar builder/fluent/whatever-it's-called pattern. Probably the most idiomatic JS way to configure things is to just shove a big options object into the constructor, but the builder pattern makes sense when you've got multiple "phases" (e.g. in Prisma's case you have the select, then filter phases). This one I don't have a super strong opinion about, and it's easy enough to write wrappers in either direction.

I'll add that I thought Tau's callback-based query interface was pretty hard to use. I think it's like that because Tau supports older browsers before generators were a thing. I think Scryer's current query interface that uses Iterable is ideal, and I also think it's fair to only target "evergreen" browsers (that is, forget about Internet Explorer support -- not that it could run modern Wasm regardless 😄).

@bakaq
Copy link
Contributor Author

bakaq commented Feb 17, 2025

Thank you for all the feedback!

Speaking of imports, personally I think it's fine to stick with ESModules (also called 'esnext' build target). These days I write all my JS stuff using them.

I was worried about NodeJS, but I tried and if you bindgen with --target nodejs it actually just works! From the documentation I got the impression that it wouldn't, but that is probably because it's out of date (it probably was written when NodeJS didn't support ESM). It seems that modern NodeJS can just deal with mixing ESM and CommonJS. Deno also already works with --target deno. Amazing! This means that we already have basically all of JS working! And also, it seems I can use and abuse things like inline_js without much repercussion for modern browsers and runtimes, which will make things much easier!

@bakaq
Copy link
Contributor Author

bakaq commented Feb 20, 2025

Ok, it seems wasm bindgen doesn't let you re-export inline_js: rustwasm/wasm-bindgen#1812. That's unfortunate.

I think I will just return raw internally tagged Javascript objects (so aways with a type field) and let the wrapping library deal with converting them to and from actual classes. I think we would need a wrapper library anyway to get some more ergonomics, so we can just have this internal unstable representation (not the "official" serialization!) for now. When we settle on #2707 we can then switch to that.

@bakaq
Copy link
Contributor Author

bakaq commented Feb 20, 2025

I've added functionality to consult modules. I forgot about it initially.

@bakaq
Copy link
Contributor Author

bakaq commented Feb 20, 2025

Ok, with documentation and consulting I think this is all I have in scope for this PR and it's ready for review.

@bakaq bakaq marked this pull request as ready for review February 20, 2025 04:45
@guregu
Copy link

guregu commented Feb 20, 2025

Updated the npm package with the latest changes. I was able to get some modules to talk to each other, but the behavior is a bit odd, throwing an existence_error unless I fully qualify imported predicates. Could be that consulting into the user module is a bad idea, but I wasn't able to access the modules from a query without doing that. Example here. Apologies if I made a mistake somewhere.

@bakaq
Copy link
Contributor Author

bakaq commented Feb 20, 2025

Ok, I experimented with the Rust API and it's indeed a mess. Consulting modules is currently completely borked and often just panics. I don't think this is a complete deal breaker for this MVP though, as just using user (aka no modules) does work fine, even with libraries.

Thanks a lot for finding these problems! I will address them in the Rust API in separate PRs, which will then automatically fix them here too.

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.

3 participants