attach metadata to handlers and allow filtering #125
Conversation
Pull Request Test Coverage Report for Build 18300062656Details
💛 - Coveralls |
|
Hmm might need to try harder on tests |
| workflow_name: str | ||
| status: Status | ||
| ctx: dict[str, Any] | ||
| handler_metadata_json: str | None = Field(default=None) |
There was a problem hiding this comment.
why the nested json? Can't this just be a dict[str, Any] (and default to {})?
There was a problem hiding this comment.
Mostly ease of storage lol but yes, good point
| type: string | ||
| enum: [running, completed, failed, cancelled] | ||
| description: Filter by handler status | ||
| - in: query |
There was a problem hiding this comment.
might make sense to make this consistent with the agent data query DSL https://developers.llamaindex.ai/python/cloud/llamaagents/agent-data-overview/#filter-dsl
I don't love that DSL, but there's not a lot of great options that don't explode in complexity. This DSL looks actually pretty similar? I would like to eventually move that agent data into the open source llamactl (or here?), so developing it out consistently should make it more re-usable and facilitate easier integration.
One difference here is that one is just using a POST to not have to get query parsing involved.
There was a problem hiding this comment.
we're already parsing json in the query. Could just continue that pattern with the other dsl?
|
|
||
| # Filter handlers | ||
| items = [] | ||
| for wrapper in self._handlers.values(): |
There was a problem hiding this comment.
this isn't really supporting completed workflows 😢 . Generally this querying seems like it should be pushed down into the persistence layer, at least in the long term, and we should push some of the state here in the server to an "in memory" persistence layer
There was a problem hiding this comment.
Oh dang, yea, once workflows complete they get removed from this list
| conn = sqlite3.connect(self.db_path) | ||
| conn.execute( | ||
| "CREATE TABLE IF NOT EXISTS handlers (handler_id TEXT PRIMARY KEY, workflow_name TEXT, status TEXT, ctx TEXT)" | ||
| "CREATE TABLE IF NOT EXISTS handlers (handler_id TEXT PRIMARY KEY, workflow_name TEXT, status TEXT, ctx TEXT, handler_metadata_json TEXT)" |
There was a problem hiding this comment.
yeah, this will need a little mini migration, otherwise existing databases will crash and not support metadata. I like this for a no-dependency sqlite version migration pattern:
| assert "Event data is required" in response.text | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
seems like we need a test for querying?
|
Maybe this question looks dumb, but why do we need to allow |
Initial stab at attaching arbitrary metadata to handlers
Concerns