Skip to content
This repository has been archived by the owner on Dec 11, 2024. It is now read-only.

Introduce TypeAnnotatedValue #24

Merged
merged 43 commits into from
Mar 27, 2024
Merged

Introduce TypeAnnotatedValue #24

merged 43 commits into from
Mar 27, 2024

Conversation

afsalthaj
Copy link
Contributor

@afsalthaj afsalthaj commented Mar 26, 2024

  • Introduce TypeAnnotatedValue (a great inspiration from wasm_wave::value::Value) to provide a convenient way to inspect the result of worker invocation. Also, we can obtain wasm_wave::value::Value directly from TypeAnnotatedValue anytime. Just note that wasm_wave::value::Value doesn't handle resource handles for now. Therefore, we are not touching this part yet.

Here is a practical use-case where this will be useful.
In golem-cloud, a match expr such as the below one can be easily evaluated to TypeAnnotatedValue. Meaning evaluation of Expr can go from TypeAnnotatedValue to TypeAnnotatedValue

match worker.response { 
   ok (v) => some(v)
   Err(msg) => none
}

And the evaluation goes as:

let worker_response: TypeAnnotatedValue = ???

match worker_response {
  TypeAnnotatedResult::Result(result_value) =>  match result_value.value {
       Ok(value) => TypeAnnotatedResult::Option(Some(value))
       Err(msg) =>  TypeAnnotatedResult::Option(None)
  }
}

This otherwise is quite fragile and clunky if we were to inspect through textual/json format.

  • A type-annotated-value can be converted to wasm_rpc::Value using From instance.

  • A type-annotated-value can also be turned into serde_json::Value keeping all our existing functionalities backward compatible. i.e, functions in current golem open source services will stay as it is. In the worker-bridge side, you can use simply invoke worker function that returns the ProtoVal version (instead of json) and use template service to get the template metadata that holds the Vec<FunctionResult>, and with these two you can form TypeAnnotatedValue using the functionalities in wasm_rpc library. Note that worker-bridge is already part of worker-service-base, so it should be an easy thing to add a new function there and worker-bridge being able to use it.

  • json::validate_function_result function still returns a JsonValue as before, however internally everything is computed in terms of TypeAnnotatedValue. That is, the actual validation is more cleaner now, but there is a cost involved here with intermediate representation when users need json .

  • Given there are already good test coverage already for this validations to json, and given obtaining JsonValue is going through the TypeAnnotatedValue logic, the existing tests are the proof the whole changes work.

  • There is also an instance of WasmValue for TypeAnnotatedValue in text module making to-and-from wave string for TypeAnnotatedValue.

  • We can also get WitValue from TypeAnnotatedValue.

@vigoo
Copy link
Contributor

vigoo commented Mar 26, 2024

TypeAnnotatedValue is placed along with Value in lib.rs. May be this goes to text module. I am still thinking.
So, it is only useful for two things: conversion to/from JSON and WAVE's text format, because for both we need not just the value but also the type information. Because it's used by both, I would not put it in either the json nor the text packages, but in the crate root.

But, as you probably have seen, the library has several cargo features:

[features]
default = ["host"]
host = ["arbitrary", "bincode", "json", "protobuf", "serde", "text", "typeinfo", "wasmtime"]
arbitrary = ["dep:arbitrary"]
bincode = ["dep:bincode"]
json = ["dep:serde", "dep:serde_json", "dep:bigdecimal", "typeinfo"]
protobuf = ["dep:bincode", "dep:serde", "dep:prost"]
serde = ["dep:serde"]
stub = []
text = ["wasmtime", "dep:wasm-wave"]
typeinfo = ["dep:golem-wasm-ast"]
wasmtime = ["dep:wasmtime", "typeinfo"]

These are important to reduce the huge amount of dependencies in various use cases, but most importantly to not have all these dependencies when it is used in the generated RPC stubs.

TypedAnnotatedValue is not required for the stubs, so it must belong to a feature that is only enabled by the host feature (and not stub).

I think the existing typeinfo feature is a good fit. That's the one that brings in golem-wasm-ast as a dependency, which contains the AnalysedType type.

So to sum it up:

  • put it in the crate root
  • but behind the typeinfo feature flag

The validation_function_result_to_typed_value exists now in json package. But I think this can exist probably in text module itself along with the type, and json module deals with only things that are related to serde_json::Value,

The json package exported two functions before:

  • function_parameters takes a JSON value and a function's type information and gets back a list of parameter Values
  • function_result takes a list of result Values and a function's type information and gets back a JSON

I think we want the same functionality for the WAVE text format, and I assume that's already in the PR (will check after this comment).

I would do the following:

  • Make the main logic of function_parameters and function_result work on TypeAnnotatedValue and these functions should be probably part of TypeAnnotatedValue's impl
  • We should keep the same two exported functions in the json package but using this implementation
  • And have an equivalent version of both in the text package that instead of JSON objects works on Strings (of the WAVE format)

Should we consider TypeAnnotatedValue to be part of grpc proto? I didn't do this now, since I didn't find it as mandatory now.

I don't think we need a protobuf version of it. We already have protobuf values of Value and Type and we should be able to reconstruct a TypedAnnotatedValue from those.


#[cfg(not(feature = "host"))]
#[cfg(feature = "stub")]
pub use bindings::golem::rpc::types::{NodeIndex, RpcError, Uri, WasmRpc, WitNode, WitValue};

#[cfg(feature = "host")]
use ::wasmtime::component::bindgen;
use golem_wasm_ast::analysis::{AnalysedResourceId, AnalysedResourceMode};
use wasm_wave::wasm::{WasmType, WasmValue, WasmValueError};
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't let your IDE do these imports, it breaks the feature flags. Either import under a feature condition, or use fully qualified names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (I think) - will not resolve conversation until you verify. Thanks

wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
wasm-rpc/src/text.rs Outdated Show resolved Hide resolved
wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
@afsalthaj afsalthaj marked this pull request as ready for review March 27, 2024 13:01
Copy link
Contributor

@vigoo vigoo left a comment

Choose a reason for hiding this comment

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

Looks nice, I'm just asking one more restructuring and maybe a parameter type change then we can merge!

wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
wasm-rpc/src/text.rs Outdated Show resolved Hide resolved
wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
wasm-rpc/src/text.rs Outdated Show resolved Hide resolved
wasm-rpc/src/lib.rs Outdated Show resolved Hide resolved
@afsalthaj afsalthaj merged commit 2267ce9 into main Mar 27, 2024
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants