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

Invocation state proxy for Function Executors and new API for functions #1134

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

eabatalov
Copy link
Contributor

@eabatalov eabatalov commented Dec 20, 2024

We don't want to have Indexify Server API credentials in Function Executors because it's a security risk if Function Executors run not trusted code. To achieve that Executor provides a gRPC API to Function Executor to get/set graph invocation state. Once a request is recieved Executor finds invocation ID of the requesting Function Executor and does Indexify Server API call to get/set and invocation state key.

There are a few customer function facing API changes here:

Instead of calling get_ctx().set_state_key() and get_ctx().get_state_key() they call
get_ctx().invocation_state.get() and
get_ctx().invocation_state.set().

This intoduces a separate API for graph invocation state which is more clear than adding unbounded number of methods into GraphInvocationContext object returned by get_ctx(). We're thinking about long term extensibility of it here. Also as local graph uses LocalInvocationState implementation and remote graph is using ProxiedInvocationState we'll just need to forward get_ctx().set_state_key() and get_ctx().get_state_key() calls into these implementations anyway.

Another change is that set(key, value) now accepts any value that is serializable using CloudPickle. This is provides a consisten "no surprises" UX because we're using CloudPickle for function inputs and outputs. Supporting this required small changes on Indexify Server side as previous JSON was explicitly used at HTTP protocol and storage layers. Now it supports arbitrary binary and textual formats for invocation state values.

Finally get(key) now returns Optional[Any] so None is returned if the key wasn't set yet. This allows the customer code to decide what to do in this case.

Testing:

make fmt
make test

Contribution Checklist

  • If the python-sdk was changed, please run make fmt in python-sdk/.
  • If the server was changed, please run make fmt in server/.
  • Make sure all PR Checks are passing.

@eabatalov eabatalov requested a review from diptanu December 20, 2024 21:55
@eabatalov eabatalov force-pushed the eugene-function-executor-key-value-api branch from f0b1267 to ff6c247 Compare December 20, 2024 23:05
We don't want to have Indexify Server API credentials in Function Executors
because it's a security risk if Function Executors run not trusted code.
To achieve that Executor provides a gRPC API to Function Executor to
get/set graph invocation state. Once a request is recieved Executor
finds invocation ID of the requesting Function Executor and does
Indexify Server API call to get/set and invocation state key.

There are a few customer function facing API changes here:

Instead of calling `get_ctx().set_state_key()` and
`get_ctx().get_state_key()` they call
`get_ctx().invocation_state.get()` and
`get_ctx().invocation_state.set()`.

This intoduces a separate API for graph invocation state which is more
clear than adding unbounded number of methods into object returned
by `get_ctx()`.

Another change is that `set(key, value)` now accepts any value
that is serializable using CloudPickle. This is provides a consisten
"no surprises" UX because we're using CloudPickle for function inputs
and outputs. Supporting this required small changes on Indexify Server
side as previous JSON was explicitly used at HTTP protocol and storage
layers. Now it supports arbitrary binary and textual formats for
invocation state values.

Finally `get(key)` now returns Optional[Any] so None is returned
if the key wasn't set yet. This allows the customer code to decide what
to do in this case.

Testing:

make fmt
make test
@eabatalov eabatalov force-pushed the eugene-function-executor-key-value-api branch from ff6c247 to 928d9fe Compare December 20, 2024 23:19
@eabatalov eabatalov marked this pull request as ready for review December 23, 2024 11:19
@eabatalov eabatalov merged commit d867689 into main Dec 23, 2024
8 checks passed
@eabatalov eabatalov deleted the eugene-function-executor-key-value-api branch December 23, 2024 12:41
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.

1 participant