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

Support context propagation #925

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support context propagation #925

wants to merge 2 commits into from

Conversation

goccy
Copy link
Contributor

@goccy goccy commented Apr 18, 2024

context.Context instance passed in ContextEval() can be propagated to binding function to cancel the process.

First of all, thanks for the great OSS. We are using cel-go heavily in https://github.com/mercari/grpc-federation .
In our OSS, we may want to pass context.Context against bound functions. e.g.) pass to the gRPC metadata or perform a cancel.

Now that the ContextEval function is provided, so I fixed to the context.Context instance is passed to the bound function.

Although I have tried to keep the Interface as unchanged as possible, I have made some changes to the Public Interface. If there is a problem, please let me know so that I can correct it.

ref #557

Pull Requests Guidelines

See CONTRIBUTING.md for more details about when to create
a GitHub Pull Request and when other kinds of contributions or
consultation might be more desirable.

When creating a new pull request, please fork the repo and work within a
development branch.

Commit Messages

  • Most changes should be accompanied by tests.
  • Commit messages should explain why the changes were made.
Summary of change in 50 characters or less

Background on why the change is being made with additional detail on
consequences of the changes elsewhere in the code or to the general
functionality of the library. Multiple paragraphs may be used, but
please keep lines to 72 characters or less.

Reviews

  • Perform a self-review.
  • Make sure the Travis CI build passes.
  • Assign a reviewer once both the above have been completed.

Merging

  • If a CEL maintaner approves the change, it may be merged by the author if
    they have write access. Otherwise, the change will be merged by a maintainer.
  • Multiple commits should be squashed before merging.
  • Please append the line closes #<issue-num>: description in the merge message,
    if applicable.

- context.Context instance passed in ContextEval can be propagated to binding function to cancel the process.
@goccy
Copy link
Contributor Author

goccy commented Apr 18, 2024

I've confirmed that all tests pass. Please review this 🙏 .

@TristonianJones
Copy link
Collaborator

/gcbrun

@TristonianJones
Copy link
Collaborator

@goccy I'll give it a look. If there are changes in the top-level APIs, I'll probably ask for parallel implementations for things like unaryOpContext, but I did a quick first pass and have a good gist of what you're aiming for

@TristonianJones TristonianJones self-requested a review April 18, 2024 17:11
@TristonianJones
Copy link
Collaborator

@goccy looks like there were some failures in the sub-modules:

Step #1 - "bazel-test": repl/evaluator.go:235:14: cannot use func(v ref.Val) ref.Val {…} (value of type func(v ref.Val) ref.Val) as type functions.UnaryContextOp in struct literal
Step #1 - "bazel-test": repl/evaluator.go:240:14: cannot use func(lhs ref.Val, rhs ref.Val) ref.Val {…} (value of type func(lhs ref.Val, rhs ref.Val) ref.Val) as type functions.BinaryContextOp in struct literal
Step #1 - "bazel-test": repl/evaluator.go:245:14: cannot use l.impl (variable of type functions.FunctionOp) as type functions.FunctionContextOp in struct literal

@goccy
Copy link
Contributor Author

goccy commented Apr 19, 2024

@TristonianJones Thank you for your quickly response. I've fixed that build error and I confirmed passing all tests by bazel test //...

@goccy
Copy link
Contributor Author

goccy commented Apr 24, 2024

@TristonianJones
If breaking changes to the interface of interpreter.Interpretable or interpreter.Qualifier etc are not allowed, would it be appropriate to add the following APIs for context to each ?

 type Interpretable {
   Eval(activation Activation) ref.Val
+  EvalContext(ctx context.Context, activation Activation) ref.Val
 }

 type Qualifier {
   Qualify(vars Activation, obj any) (any, error)
   QualifyIfPresent(vars Activation, obj any, presenceOnly bool) (any, bool, error)
+  QualifyContext(ctx context.Context, vars Activation, obj any) (any, error)
+  QualifyIfPresentContext(ctx context.Context, vars Activation, obj any, presenceOnly bool) (any, bool, error)
 }

Also, should I add the fields for context interface to the functions.Overload ?

@TristonianJones
Copy link
Collaborator

@goccy Sorry for the delayed reply. I've been weighing this approach with a previous one we refer to as iterative-eval. The idea behind iterative eval is that context dependent functions are initially stubbed with synchronous versions which capture arguments and return unknown. Then a resolve step would happen to execute the functions containing Context arguments, cache the results, and re-evaluate. This process repeats until there are no more unknowns to resolve.

The benefit of such an approach is that it's async in terms of a synchronous process and that it allows for batching of async calls. The drawback is that it's somewhat complex and doesn't completely fit with user expectations regarding which functions are executed when.

Part of the reason we haven't adopted either approach is because of the plumbing changes required to support them. Effectively, you need parallel methods to the ones that exist today. Since I'm not sure who might be using the Attribute interface, introducing new methods on that interface could be a breaking change for large projects due to golang's minimum version selection.

I'll need to think about this more. Thanks for your patience!

@goccy
Copy link
Contributor Author

goccy commented Apr 25, 2024

@TristonianJones Thank you for your explanation ! To avoid breaking changes, what about the following approach of defining a new interface and using it only internally? If this looks good, I will try to implement it.

type InterpretableContext interface {
  Interpretable
  EvalContext(ctx context.Context, activation Activation) ref.Val
}

type QualifierContext interface {
  Qualifier
  QualifyContext(ctx context.Context, vars Activation, obj any) (any, error)
  QualifyIfPresentContext(ctx context.Context, vars Activation, obj any, presenceOnly bool) (any, bool, error)
}

@goccy
Copy link
Contributor Author

goccy commented May 15, 2024

@TristonianJones

I apologize for the additional mention, but I've implemented this without any breaking changes. I would appreciate it if you could review it when you have time.

#943

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.

2 participants