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

Fixes #966, #797 and other clean ups #987

Merged
merged 15 commits into from
Oct 3, 2024

Conversation

afsalthaj
Copy link
Contributor

@afsalthaj afsalthaj commented Oct 2, 2024

Fixes #966, #797, and other cleanups:

  • We already have a WorkerServiceRibInterpreter service in the worker-service module, which handles the details of the worker-gateway (worker-service). To maintain consistency, this PR introduces WorkerServiceRibCompiler, responsible for compiling Rib Expr in a way specific to the worker-gateway. Example: WorkerServiceRibCompiler ensures that the only allowed global variable in the Rib scrip is a request.

  • Removed the pure_evaluate function in the worker-service module. It was slightly harder to reason about, as it primarily dealt with evaluating Rib expressions without function calls (e.g., resolving a worker-name expression). This has been replaced with directly calling the rib::interpret_pure function. This is useful anytime we need to evaluate a Rib expression anywhere in Golem.

  • The function-type registry is a map of RegistryKey to RegistryValue. RegistryKey is a FunctionName, but it included includes Enum, Variant, etc. As I was improving Rib, I find this part confusing, so I cleaned it up. To begin with, I wanted to treat everything as a generic function call and delegate the responsibility of differenting whether it is a Variant/Enum or actual worker-function call to the Rib Interpreter. However, that turned out to be a long-term goal with many brittle logic points. So it is still almost the same except that RegistryValue will hold the information whether or not it is a Variant. This allows safer pattern match and logical decisions during type inference. This adjustment allows for more context-aware error messages. Example: Instead of "wrong number of arguments to register-user," we can have "wrong number of arguments to variant register-user". Likewise, we have the flexibility to add more information in the error messages.

  • Renaming of some important variables, adding an fixing comments making it easier to understand the workflow of serving a custom http request for a new developer.

  • Early error messages for invalid function calls before going to other phases of compilation which makes the error message more opaque due to extra details.

Unknown function call: `foo`
Unknown resource constructor call: `golem:it/api.{cart0(user_id).add-item}`. Resource `cart0` doesn't exist
Unknown resource method call golem:it/api.{cart(user_id).foo}. `foo` doesn't exist in resource `cart`
Incorrect number of arguments for function `foo`. Expected 1, but provided 2
Incorrect number of arguments for resource constructor `cart`. Expected 1, but provided 2
Invalid type for the argument in resource method `golem:it/api.{cart(user_id).add-item}`. 
Expected type `record`, but provided argument `\"foo\"` is a `str`

etc

@afsalthaj afsalthaj changed the title Fixes #966 and other clean ups Fixes #966, #797 and other clean ups Oct 2, 2024
@afsalthaj afsalthaj marked this pull request as ready for review October 2, 2024 11:14
@afsalthaj
Copy link
Contributor Author

The error messages in the PR description is a candidate for review

@@ -226,7 +229,44 @@ mod internal {
}
}

_ => {}
Copy link
Contributor Author

@afsalthaj afsalthaj Oct 2, 2024

Choose a reason for hiding this comment

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

As I do further tickets, I will ensure this _ is removed across the code. In this PR, I have fixed a few of them. Even if it results in some duplicate looking code, it ensures correctness and accidentally missing out on required cases.

"NoopWorkerRequestExecutor".to_string(),
))
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no need of using NoopWorkerRequestExecutor anywhere in code. It's dangerous.

Some(vec!["request".to_string()]),
)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make it more consistent with WorkerServiceRibInterpreter

&self,
expr: &RibByteCode,
rib_input: &RibInputValue,
) -> Result<RibInterpreterResult, EvaluationError>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

evaluate_pure is not a concern of worker-service.

@afsalthaj afsalthaj merged commit a424cb9 into main Oct 3, 2024
17 checks passed
@afsalthaj afsalthaj deleted the remove_enum_and_variant_from_type_registry branch October 3, 2024 12:40
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.

Avoid Enum and Variant types to be part of type_registry
2 participants