- 
                Notifications
    You must be signed in to change notification settings 
- Fork 8
Improve kernel constructor to respect kernelspec args #16
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
Improve kernel constructor to respect kernelspec args #16
Conversation
| Talking about the design here as this ends up being the middleware. 
 i) Xeus-python and Xeus-r's wasm-interpreter technically doesn't need anything from the kernel.json. So for those cases we would just use the basic constructor which is  ii) But for xeus-cpp's case the kernel.json can influence the interpreter being built. So in those case we use  | 
| How do we decide what kind of wasm_interpreter should be used ? a) We check the kernel.json length (it should be more than 1 as the 1st element is the binary itself) This is what xeus-cpp has been doing (a separate wasm_kernel.json than the native one) 
 
 Quite some things like the debugger and the connection file don't make sense here. Hence we should have wasm_kernel.json just like xeus-cpp has and this should probably end up being So yeah as can be seen if the length of argv is greater than 1, then we know we have some arguments to take care of ! | 
| Would require some changes in xeus-python (and other kernels) for example just adding the constructor . We already have this We also need this to compile  | 
| As discussed have made the change to simply pass a vector of strings to the wasm interpreter. Would have been great if xeus provided a "build_interpreter" or some helper (that can then be overriden by downstream kernels as per whatever usecase) I don't think I could find one. So I've just passed it over the constructor for now ! The relevant commit to get this working with xeus-cpp is this compiler-research/xeus-cpp@3ffa196 | 
| Also not sure why the CI doesn't run ! | 
| IIRC the ubuntu version being used was deprecated you need to update it or use  | 
2dc4968    to
    01ac8b3      
    Compare
  
    fd1f416    to
    dc1adad      
    Compare
  
    | 
 I'd be 👍🏽 for that. I have slight preference for this approach over changing the interpreter ctor signature. | 
| Actually the helper does not have to be in xeus::core. Such a building callback (like we have for the xsever, or the debugger) makes sense when it receives arguments from the caller; either because the arguments were initially passed to the caller, or because the caller built them. But here the arguments are passed from xeus_lite, not xeus core, and then a callback in xeus core would be a function taking no arguments; and xeus-lite would have to pass a lmabda capturing its argument which oes not look right to me. Another solution would be to pass a callback to  template <class interpreter_type>
struct default_builder
{
    interpreter_type* operator()(ems::val /*js_argv*/) const
    {
        return new interpreter_type();
    }
};
template<class interpreter_type>
std::unique_ptr<xkernel> make_xkernel(interpreter_type* interp)
{
    // ...
}
template <class interpreter_type>
void export_kernel(const std::string kernel_name, std::function<interpreter*(ems::val)> bd = default_builder<interpreter_type>{})
{
    // ...
            ems::class_<xkernel>(kernel_name.c_str())
                .constructor<>([&bd](ems::val js_argv = {}) { return make_xkernel(bd(js_argv)); })
                . // ....
};I think this change wuld be backward compatible, unless some kernels make direct use of the  | 
| Hmmm, I tried fitting in this design in some way possible without success :( I realize that the constructor provided by embind isn't catering to a lot of usecases. For starters I haven't seen a constructor (on emscripten docs/github) having a constructor body Firstly I confirmed with an embind maintainer that there is no binding code to handle capturing lambdas. So something like this can't be put to use Secondly I see constructors only being put to use in 2 ways (https://emscripten.org/docs/porting/connecting_cpp_and_javascript/embind.html#external-constructors) i) Either stick to what's been done for the Class implemented on the C++ side We are using ii) as of now (factory function is make_xkernel, args is passed from jupyterlite-xeus) | 
| Ok, a more "restrictive" solution (because it won't support lambdas) can be to pass a function pointer as a template parameter: template <class interpreter>
using builder_type = interpreter* (*)(ems::val);
template <class interpreter>
interpreter* default_builder(ems::val)
{
    return new interpreter();
}
template <class interpreter>
interpreter* builder_with_args(ems::val js_args)
{
    return new interpreter(js_args);
}
template<class interpreter_type,  builder_type<interpreter> builder>
std::unique_ptr<xkernel> make_xkernel(ems::val js_args)
{
    // ...
    auto* inter = (*builder)(js_args);
    // ...
}
template<class interpreter, builder_type<interpreter> builder = &default_builder<interpreter>>
void export_kernel(const std::string kernel_name)
{
        ems::class_<xkernel>(kernel_name.c_str())
            .constructor<>(&make_xkernel<interpreter_type, builder>)
            // ...
}And from the kernel: export_kernel<my_interpreter_type, &builder_with_args<my_interpreter_type>>("magics"); | 
| The above comment #16 (comment) works very smoothly. | 
Screen.Recording.2025-04-17.at.1.47.31.PM.mp4
Demo above : Shows 3 xeus-cpp-lite kernels built (all use different flags
-std=c++17/20/23)On top of which xcpp20 is built using the -msimd128 flag (required for running xsimd in the browser)
Basically using
So xcpp20 kernel would work but xcpp23 wouldn't !