Conversation
|
|
| // Internal hooks used by macro expansions | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| def registerPlan[Trait]( |
There was a problem hiding this comment.
What exactly the term “Plan” here mean conceptually? Just because I didn’t use the term plan anywhere in TS or Rust.
There was a problem hiding this comment.
If you don't like the naming we can change it. "Plan" exists because the Scala runtime needs a single explicit value (AgentImplementationPlan) that captures an agent’s metadata + method routing + codecs + handlers so it can register and execute the agent uniformly at runtime.
| import java.util.concurrent.TimeoutException | ||
| import scala.sys.process._ | ||
|
|
||
| object GolemPlugin extends AutoPlugin { |
There was a problem hiding this comment.
Both this sbt plugin, and the mill one seem to do a lot of work to wrap golem-cli as commands and build steps, so sbt or mill is the outer build tool driven the packaging and deployment of a golem app. We should not do this, the CLI is not intended to be used like this - Golem's app manifest itself contains build steps and language specific profiles for these, and the Scala integration would be a new golem-cli template invoking sbt (or mill) under the hood to compile the Scala code to a single final JS file (just like the TS template is using rollup to do that).
There are many features in golem-cli which all assumes that it is the "driver" and not the other way around.
There was a problem hiding this comment.
In the JS example I've sent to you I commented this in the golem.yaml: https://github.com/vigoo/js-counter-agent/blob/master/golem.yaml#L5-L12
The examples in this repo should all have a golem.yaml having a scala build template with a build step compiling Scala to JS, and the other steps for wrapping it in a Golem WASM untouched. Once this library is published we will put this build template in golem-cli
golem/README.md
Outdated
|
|
||
| 1. **`golem-cli`** installed and on your `PATH`: | ||
| ```bash | ||
| curl -sSf https://join.golem.network/as-requestor | bash - |
There was a problem hiding this comment.
is that the expected bash script? security wise, it looks troublesome
There was a problem hiding this comment.
Golem Network is not our thing, this must have been generated here by some AI
There was a problem hiding this comment.
The real one: https://learn.golem.cloud/cli#installation
There was a problem hiding this comment.
Yeah, that's my bad. I had originally done some AI generation for the initial boilerplate and this never got removed when I cleaned up. I'll get it cut.
There was a problem hiding this comment.
This should be fixed and I've updated all the docs to properly represent the latest code.
| */ | ||
| val golemExports = | ||
| settingKey[Seq[GolemExport]]( | ||
| "List of exported agents (Scala-only); used to auto-generate a hidden BridgeSpec manifest." |
There was a problem hiding this comment.
I don't understand what the build plugins are doing. They seem to analyse the compiled byte code to have their own concept of exported agents and then generate TypeScript code and embed that generated TypeScript code together with the Scala.js output into a TypeScript Golem app?
This is absolutely not how it should work. The TypeScript SDK was just provided as an example of what to do but in Scala. The compilation should result in a single JS file that have the proper exports (as shown in the hand-written example https://github.com/vigoo/js-counter-agent/blob/master/main.js) - and these exports (invocation, agent metadata etc. should be implemented by the SDK based on the Scala macros that somehow register the annotated classes as agents.
There was a problem hiding this comment.
The plugins have been completely removed as they're no longer necessary.
|
Before digging into reviewing the details, I would like to discuss the architecture because I think there are fundamental differences between how we planned Scala.js integration into Golem and what the PR does. I tried to point out a few things regarding this in comments above, but it is hard to understand the whole picture; Could you write me a summary of how the current proposed SDK works (under the hood - the user examples look nice). I would like to understand all the steps involved from annotating a class - what the macros does and why do the build plugins do additional steps, and so on. Having that high level overview would help to figure out how to align this with what we need. |
| * `@agentDefinition("...")` on the trait. | ||
| */ | ||
| def register[Trait](constructor: => Trait): AgentDefinition[Trait] = | ||
| macro AgentImplementationMacroFacade.registerFromAnnotationImpl[Trait] |
There was a problem hiding this comment.
If registerFromAnnotationImpl is all about registering using a custom agent type name then probably lets rename to registerImplCustomAgentTypeName or something that reflects that idea.
|
|
||
| import scala.concurrent.Future | ||
|
|
||
| final case class AgentImplementationPlan[Instance]( |
There was a problem hiding this comment.
So does this inherently represent singleton agent instances? That there are no constructors, or in other words agent input is some unit?
There was a problem hiding this comment.
It is a no-args durable instance if that's what you mean?
There was a problem hiding this comment.
FYI: We call these instances that don't take any constructor arguments as "singleton agents".
Why?: With zero constructor arguments, there is no way there can be more than 1 instance of this agent unless they are created through "phantom" functions (note, each instance of this agent has distinct agent-ids, which is composed of constructor argument values along with agent-type name.
|
|
||
| /** | ||
| * Agent implementation plan where the agent instance depends on constructor | ||
| * input. |
There was a problem hiding this comment.
I think, if possible, we should simplify this idea. If the constructors don't exist, it's just empty. Having to separately handle them using different types feels like unnecessary.
In other words, AgentImplementationPlan should never be a separate type. It is already covered in AgentImplementationPlanWithCtor. And thereby rename AgentImplementationPlanWithCtor[Instance, Ctor] to just AgentImplementationPlan[Instance, Ctor]
| ) | ||
|
|
||
| object ClientMethodPlan { | ||
| type Aux[Trait, In, Out] = ClientMethodPlan[Trait, In, Out] |
There was a problem hiding this comment.
Are we sure this Aux is required ? Trait, in and Out are already input type parameter and not type members.
| * This exists purely for Scala ergonomics: user agent traits can extend | ||
| * `BaseAgent` without importing any packaging/runtime-specific concepts. | ||
| */ | ||
| trait BaseAgent |
There was a problem hiding this comment.
May be in scala, this workflow won't work and that's why you may have chose this approach and holding the above functionalities somewhere else. Could you confirm it?
In other SDKs, BaseAgent is quite an important base trait holding mainly three functions along with a few others. Those 3 important functionalities are
- Get the AgentID
- The dynamic method router - given a method name, and arguments as
data-value, delegate it to the real static implementation and return a data value - Get the agent-type.
There was a problem hiding this comment.
Yes, I'm deliberately not modeling those three functions because they live in the guest exports + registry/definition/binding runtime instead
There was a problem hiding this comment.
Thanks for patiently addressing most of the comments. Really appreciate it.
However, with this PR comment, I was personally looking forward to a solution. I kind of get your explanation but not fully.
My point was when an agent trait extends BaseAgent, "registry/definition/binding" is essentially ensuring they implement these functionalities in BaseAgent. Regardless of the language, I would wish BaseAgent has everything other than the actual agent-methods, an Agent can do. Example: getId. This is not something user write in their agent trait, but they get it. When they annotate their agent-implementation, this getId will now have an implementation. otherwise, it won't compile. Now you can see the maintainaibility of this SDK that we developers can easily extend things in BaseAgent and work backwards to satisfy the compiler (when it comes to Rust and Scala, and to some extent TS too)
For example: It was easy for @vigoo to add load_snapshot and save_snapshot (like here) into the BaseAgentand work backwards (atleast for the most part, and then he changed things in macros). I am not saying that's how everyone should do, but it will be really easy for both maintainers and users and see what's going on. In this case, users see they are extending BaseAgent but, they don't see anything in it.
There was a problem hiding this comment.
I've updated the code so agentId, agentType, and agentName are all methods in BaseAgent now that simply invoke BaseAgentPlatform. Does that resolve this concern?
| .collectFirst { | ||
| case p if p.metadata.name == $methodName => | ||
| p.asInstanceOf[_root_.cloud.golem.runtime.plan.ClientMethodPlan.Aux[$traitTpe, $inTpe, $outTpe]] | ||
| } |
There was a problem hiding this comment.
Could you confirm this casting is sound? With traitTpe , Input and Output?
There are no runtime witnesses. scala/jvm erases it. So probably the reasoning here would be somehow just satisfy the compiler, but it also looks odd, that we ended up having to do this. An explanation would suffice, not asking to immediately change.
There was a problem hiding this comment.
Yes, this is just to satisfy the compiler, but the signature has changed somewhat with the removal of Aux.
| * Registers an agent implementation using the agent type name from | ||
| * `@agentDefinition("...")` on the trait. | ||
| */ | ||
| inline def register[Trait](inline constructor: => Trait): AgentDefinition[Trait] = |
There was a problem hiding this comment.
Let's rename to inline build: => Trait instead of inline constructor : => Trait
| * `@mode(DurabilityMode.Durable)`) or defaults to Durable when not specified. | ||
| */ | ||
| inline def register[Trait <: AnyRef { type AgentInput }, Ctor](inline build: Ctor => Trait): AgentDefinition[Trait] = | ||
| registerWithCtorInternal[Trait, Ctor](AgentNameMacro.typeName[Trait])(build) |
There was a problem hiding this comment.
I kind of understand the reasoning behind branches of with constructor and without constructor in the internals. Probably to allow user to avoid writing type AgentInput = ... I have mixed feelings about this, because singleton types are not quite common in golem.
Definitely not a bad idea though.
So I am ok type AgentInput =... is an optional thing at the user level, however, we should try avoiding (if we haven't already) separate separate logic/code-path-ways (such as two different types mention in another PR comment of mine, two different registration function like the ones here) in the internals.
There was a problem hiding this comment.
This should also be resolved with the previous merging of AgentImplementation
|
|
||
| object ShardModule { | ||
| val definition: AgentDefinition[Shard] = | ||
| AgentImplementation.register[Shard, (String, Int)](in => new ShardImpl(in)) |
There was a problem hiding this comment.
I hope user never need to manually register the agent implementation in the code. :)
| * Generates a TypeScript bridge (`src/main.ts`) compatible with the `component new ts` shape, but driven by a | ||
| * user-provided {@link BridgeSpec} (no hard-coded example agents). | ||
| */ | ||
| public final class TypeScriptBridgeGenerator { |
There was a problem hiding this comment.
I am not sure about this bridge generator. Trying to understand why you ended up having it :)
I think this goes back to what @vigoo already commented here: #554 (comment)
I think, let's resolve this confusion first before you address rest of the code level reviews. I will continue to review code level afterwards
There was a problem hiding this comment.
Nice 👍 because this was the most confusing part for me.
| sym.name | ||
| .replaceAll("([a-z0-9])([A-Z])", "$1-$2") | ||
| .replaceAll("([A-Z]+)([A-Z][a-z])", "$1-$2") | ||
| .toLowerCase |
There was a problem hiding this comment.
In scala-3, this is kebab case. In scala-2 the same macros exist, but there I think default to the original trait name for agent type I guess. For this function defaultTypeNameFromTrait
| val traitSym = traitTpe.typeSymbol | ||
|
|
||
| def defaultTypeNameFromTrait(sym: Symbol): String = | ||
| sym.name.decodedName.toString |
There was a problem hiding this comment.
Inconsistency here between scala-2 and scala-3. I am personally not so worried about scala-2, yet, let's get rid of all inconsistencies.
There was a problem hiding this comment.
Please resolve this concern only after I finish this round of review :)
|
|
||
| if (noneCase.isEmpty || someCase.isEmpty) None | ||
| else { | ||
| val someValue = someCase.get.value.asInstanceOf[Reflect.Bound[Any]] |
There was a problem hiding this comment.
Scaringly ttoo many asInstanceOf in this file. I hope there is a legit reason. But its very hard to pick one by one. I kindly request you to ensure these are valid
There was a problem hiding this comment.
Not so concerned. A couple of them I managed to understand, but I still request you to take 1 more look at all of them. Not the cast to Refect.Bound[Any], but the cast to A happening elsewhere.
| case Some(p) => | ||
| value match { | ||
| case NullValue => | ||
| p.primitiveType.toDynamicValue(().asInstanceOf[A]) |
There was a problem hiding this comment.
Confusing. So in this case, we piggyback on the toDynamicValue in the trait passing as ().asInstanceOf[A]. Do you mind explaining a bit what's going on here?
| case PrimitiveValue.String(v) => StringValue(v) | ||
| case PrimitiveValue.Boolean(v) => BoolValue(v) | ||
| case PrimitiveValue.Byte(v) => IntValue(v.toInt) | ||
| case PrimitiveValue.Short(v) => IntValue(v.toInt) |
There was a problem hiding this comment.
Hmm.. Quick glance at this is concerning. Both Byte and Short widens to Int, and then in the opposite pattern match any IntValue is simply converted to PrimitiveValue Int, instead of making sure to convert specifically to PrimtiveValue.Byte or Primitive.Short? Wouldn't this fail in a round trip test?
case IntValue(v) => DV.Primitive(PrimitiveValue.Int(v))
I
| case BoolValue(v) => | ||
| DV.Primitive(PrimitiveValue.Boolean(v)) | ||
| case IntValue(v) => | ||
| DV.Primitive(PrimitiveValue.Int(v)) |
There was a problem hiding this comment.
| case Some(p) => | ||
| value match { | ||
| case NullValue => | ||
| p.primitiveType.toDynamicValue(().asInstanceOf[A]) |
There was a problem hiding this comment.
Just trying to understand. So for any Nullvalue against any primitve, we cast () as A. I think that's terribly wrong in the first glance :D If the reflect says String, but whats' really sent is Nullvalue, this produces a ClassCastException instead of some clean errors
| s"AgentImplementation.register[$traitFqn](new $implFqn())" | ||
| case tpe :: Nil => | ||
| val fqnTpe = fqn(ai.pkg, tpe) | ||
| s"AgentImplementation.register[$traitFqn, $fqnTpe]((in: $fqnTpe) => new $implFqn(in))" |
There was a problem hiding this comment.
@vigoo Just to get your opinion I am tagging you. Please take a look at the way agent-registration expression is created, and see what you think.
| case Pkg.Object(_, name, templ) => | ||
| val nextPkg = appendPkg(pkg, name.value) | ||
| templ.stats.flatMap(collect(_, nextPkg)) | ||
| case cls: Defn.Class if hasAgentImplementation(cls.mods) => |
| os.write.over(baseOut, baseContent) | ||
|
|
||
| T.log.info( | ||
| s"[golem] Generated Scala.js agent registration for $basePackage into $genBasePkg (${impls.length} impls, ${byPkg.size} pkgs)." |
There was a problem hiding this comment.
The collect method that works on a real Tree (parsed with proper dialect) gives me a decent confidence in registration functions. I think, some of the other bits such as the above baseContent is unavoidable. But let's get one final opinion from Vigoo
There was a problem hiding this comment.
I'm not saying we have to do it differently, but would like to understand fully why we need it. I guess there is no way to generate any kind of registration code macro-time and no reflection either so we need to do this post process step? Please give a short summary of why this is the way.
There was a problem hiding this comment.
The build-time codegen is necessary because:
- No runtime discovery in Scala.js/WASM - no classpath scanning, no Class.forName, no ServiceLoader, no reflection in the QuickJS guest.
- Macros are local - @agentImplementation expanding on FooImpl only sees FooImpl; it cannot discover other @agentImplementation classes across the project.
- The guest entry point needs a single registry - the WASM component must dispatch incoming RPC to all agent impls, so something must produce the registerAll() that wires them all.
- The build plugin is the only point where "all impls" is knowable - SBT/Mill scans sources (via regex), collects all @agentImplementation classes, and generates the registration file.
The three entry points (init, main, @JSExportTopLevel("__golemRegisterAgents")) exist because different Golem runtime versions discover the JS entry point differently - the @JSExportTopLevel val is primary, the others are fallbacks.
…d modularity, added support for kebab-case conversion of trait names.
…er validation of method names and return types.
|
Thanks for addressing a lot of comments. Given it takes a lot of experimental approaches to solving Scala with Golem, we believe there are chances of further improvements especially around auto-registration, examples etc. These already went through a lot of improvements from the initial state. A functionality test was performed - from simple deployments, to invoking a simple agent, RPC, and whether we have full access to golem host functionalities and if they really work or not. And it did, but that said, I have not tested every single host function and see if it works or not. The IDE experience with Scala3 & zio-golem looks fine as well. Issues of inferences related to RPC were all fixed as well. golem-ai integration looks ok, but I haven't developed a full fledged AI app with this to further verify. But I will be surprised if it doesn't work. The PR went through a lot of improvements recently, and it has got to a state, where its better maintainable than what it was before a few weeks. Whether or not its the best possible state, its very subjective. Therefore, approving from my side, and its a fantastic addition to Golem, and thanks again for all the hard work. Let's get another approval from John or Vigoo to further merge |
…or Scala 3 tests.
… commands in CI workflow.
…prove execution manageability.
This pull request introduces the initial implementation of the ZIO-Golem Scala SDK, providing a type-safe, macro-powered toolkit for building Golem agents. The changes include a comprehensive project README, new macro-based agent registration and client APIs for Scala.js, and supporting configuration files.
Documentation and project setup:
README.mddescribing ZIO-Golem's features, usage examples, project structure, build instructions, and developer tooling for sbt and Mill. This serves as the main entry point for new users and contributors..gitignorefile to exclude IDE, build, and local configuration artifacts from version control.Core agent and client APIs (Scala.js):
AgentImplementation.scala, providing macro-based registration of agent implementations with automatic derivation of RPC bindings, schemas, and metadata. Supports trait-based agent definitions and compile-time wiring.AgentClient.scala, enabling macro-based client generation for agent traits, with support for Scala.js and explicit plan resolution/binding.