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

Plugin vars #14

Closed
wants to merge 5 commits into from
Closed

Plugin vars #14

wants to merge 5 commits into from

Conversation

syke99
Copy link
Collaborator

@syke99 syke99 commented Sep 16, 2023

This PR adds convenience methods to Set and Get variables. It handles checking the native endianness internally so that users don't have to check this, as well. They can simple pass in a K/V pair where K's type is string and V's type is implemented as an any in the signature

@syke99 syke99 requested a review from mhmd-azeez September 16, 2023 14:08
@syke99 syke99 mentioned this pull request Sep 16, 2023
@zshipko
Copy link
Contributor

zshipko commented Sep 16, 2023

Nice! We have been talking about how to expose vars to the host.

My only hang-up with this is that it depends on encoding/binary package, which is super convenient for Go guests but I don't know if that encoding is available in other guest languages. Is there a way to make the encoding configurable or more generic somehow?

@syke99
Copy link
Collaborator Author

syke99 commented Sep 16, 2023

That's true, I didn't even consider that whenever implementing the encoding. As far as I can think currently, there are two, more simple, and universal encoding formats that could be utilized (given they're also supported by tooling used to build PDKs, i.e. TinyGo).

1: leave SetVar signature as is, and inside the method body, make a map[string]interface{}{ key: value } with the method params, and marshal to []byte, error via encoding/json

2: leave SetVar signature the same, but internally, use this to create a new *structpb.Value, then call .ProtoReflect().Interface() to return a protoreflect.Message and pass it to proto.Marshal() function to return []byte, error

The JSON implementation is pretty widely accepted and used, plus I know for sure that TinyGo implemented encoding/json support, so there wouldn't be an issue, at least with Go, on the PDK side retrieving and decoding the []byte. On the other hand, protobufs are more efficient than JSON in general (usually). However, I'm not certain they're supported in WASM or the tooling used to compiled plugins for different languages with extism

there are other options, of course like XML, or more obscure encoding/decoding formats, but they're not guaranteed to be supported in WASM, or even widely used. so those two options are the ones I'm leaning towards

@syke99
Copy link
Collaborator Author

syke99 commented Sep 16, 2023

However, I'm not certain they're supported in WASM or the tooling used to compiled plugins for different languages with extism

another thing about protobufs that I found while reading the docs, in Go, at least: When converting an int64 or uint64 to a NumberValue, numeric precision loss is possible since they are stored as a float64. This is referenced whenever using structpb.NewMessage() to create a protobuf.Message from a value. There's also some restrictions on exactly what the types can be (eg. int8, int16, and a couple other data types that can't be stored). So even though there's the overhead, I'm personally leaning towards a JSON implementation for serialization

This was referenced Sep 16, 2023
@wikiwong
Copy link
Contributor

wikiwong commented Sep 19, 2023

@syke99 thank you kindly for getting your ideas into PRs and the thoughtfulness along the way. Could you expand a bit on the use case in mind of allowing the host to access plugin vars? Though this is something we have considered in the past, we veered away from it to ensure that the plugin has some more guarantees about the state of the system when it executes. If the host is allowed to modify plugin variables, it could set a variable to a value that the plugin doesn't expect. Often times, plugin authors don't have any control over the host that their plugin is executing in, so not being able to have the guarantee that a plugin variable hasn't changed is a fairly significant paradigm shift. If we went this direction, there would be an implied path to have the same support in all of the other language SDKs

@syke99
Copy link
Collaborator Author

syke99 commented Sep 19, 2023

@wikiwong sure!! in fact, I have a project I've been working on that's currently private, as I plan on releasing it soon (this is actually a partial blocker for what I have in mind, though I could work around it with custom host functions; I'd just need to implement a small wrapper in all extism supported languages), but it can provide a good idea of the exact use case I have in mind. If not, I'm also happy to explain here

@syke99
Copy link
Collaborator Author

syke99 commented Sep 20, 2023

@wikiwong here, instead, I'll just drop this short description.

The mentioned project is a configurable data pipeline written in Go.

In this application (along with the cli I’ve written for it), a team owning the overall pipeline would hold a position similar to a DevOps team, where they would manipulate a configuration file to specify filepaths to specific wasm modules, global variables, the order of the steps, etc.

Teams that own steps/modules can write their modules and specify variables that they want set for each step. This will take some coordination with the team owning the pipeline as they will get specified in the configuration file (this can be done with a custom tool to separate steps into dif config files and the coalesce them together before running, cooperating with the pipeline team to verify correct variables are set, etc. it will be left up to the teams). As a side note (that’s also pointed out to users already), if variable names collide between global and step-specific variables, the the step-specific ones will take precedence.

However, this set up will allow teams to make incremental adjustments to their pipelines, reduce variables having to be set repeatedly across teams (variables that should be available to all steps can just be set globally in the config), and other benefits, such as teams being able to use their desired language for their steps (thanks to extism 😉)

@wikiwong
Copy link
Contributor

Thanks @syke99 for going into detail on the use case. It sounds like a very cool system, and I'm glad that Extism has a role in it!

Is there any scenario in which the plugin would need to modify vars, or are the vars intended to be specified in some configuration file and passed from the host to the plugin. I am wondering if Extism's configuration would suffice for your needs. https://extism.org/docs/concepts/configuration/

We are light on examples/documentation for configuration, but I can help you experiment with it if you haven't evaluated it already

@syke99
Copy link
Collaborator Author

syke99 commented Sep 20, 2023

That's actually what I had originally went with. But whenever I first started working on this project, the SDKs hadn't been separated out, and I noticed that configs only accepted a map[string]string at the time (I had to step away from development of this project due to some personal life circumstances, but have had the opportunity to slowly start picking it back up). If the new SDK can support passing more than just strings through the config, that would work.

And to answer your question about plug-ins setting variables as well, while it hasn't been implemented yet, yes the plan would be to allow the plugins to change their variables and have it persist through steps. But the host would only ever explicitly change the variables that have been set in the configuration file per step. And the idea would be to, once this functionality would be supported, provide an additional field in the configuration for a variable to be overridden whenever a step from that plugin gets called. If the override was allowed, than even if the variable exists/was set in a previous step, it would then be overridden when the next step is called. again, this would only ever be allowed through the configuration file in my case. though, i can see why it would cause hesitation for an overall implementation across all of extism to allow this

@wikiwong
Copy link
Contributor

wikiwong commented Sep 20, 2023

You are correct that Extism's configuration in go is still a map[string]string. Although this is an interesting use case, and I can see the value in persisting plugin variables across executions, I don't think we can commit to this direction at this time because of the implication to enable it for all other languages as well. We started planning our Extism 1.0 release and will be soliciting feedback from the community. I'll ensure that the ability to obtain data from the host, manipulate, and persist data across plugin executions is a topic of discussion. There may be another more abstract approach that we can consider.

We really do appreciate you taking the time to explain your use case and propose a solution, it truly does help us make Extism better! Please reach out if there's any other way we can help you achieve your goal

@syke99
Copy link
Collaborator Author

syke99 commented Sep 20, 2023

Yeah, no worries!! I completely agree and understand the concern. I can move forward with the release of my project, with a note in release documentation that configurable and persistent variables will be on its own roadmap for a future release.

I'm more than happy to have brought it up, though, and am more than happy to help out in any other way I can!!

@wikiwong
Copy link
Contributor

wikiwong commented Oct 4, 2023

Closing this one out for now with the intent to revisit at a later point based on our discussion

@wikiwong wikiwong closed this Oct 4, 2023
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