Create Genie API wrapper#2
Conversation
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
Signed-off-by: Prithvi Kannan <prithvi.kannan@databricks.com>
| } | ||
|
|
||
| def start_conversation(self, content): | ||
| resp = self.genie._api.do( |
There was a problem hiding this comment.
question, why are we using the raw _api request here? Can we not use this method directly? https://databricks-sdk-py.readthedocs.io/en/stable/workspace/dashboards/genie.html#databricks.sdk.service.dashboards.GenieAPI.start_conversation
There was a problem hiding this comment.
i think the polling mechanisms with the start_conversation API does not work. ill asked the genie team and they recommended to use the _api for now. we can revisit when fixed.
| headers=self.headers, | ||
| ) | ||
| if resp["status"] == "EXECUTING_QUERY": | ||
| sql = next(r for r in resp["attachments"] if "query" in r)["query"]["query"] |
There was a problem hiding this comment.
Are we getting the sql statements here only for the debug statement? Do end customers need this information?
There was a problem hiding this comment.
eventually these should be part of the trace. currently adding extra traces with autologged traces are not supported, but this is coming soon. we'll use the SQL at that time.
|
|
||
| return poll_result() | ||
|
|
||
| def ask_question(self, question): |
There was a problem hiding this comment.
What do you think of adding a timeout feature here, so users are not stuck waiting forever in the polling loop?
There was a problem hiding this comment.
good question. i wonder if genie requests have some RPC level timeout on that side, but i think a client side timeout also makes sense. will update
There was a problem hiding this comment.
actually genie has a CANCELED state that happens in the case of timeout, which will hit the else case and break the loop. i dont think we need the client side timeout.
| logging.debug(f"SQL: {sql}") | ||
| return poll_query_results() | ||
| elif resp["status"] == "COMPLETED": | ||
| return next(r for r in resp["attachments"] if "text" in r)["text"]["content"] |
There was a problem hiding this comment.
For genie, do we only care about the first response in the list?
There was a problem hiding this comment.
at this time there's only one text attachment from genie so this is a safe assumption.
| {"status": "COMPLETED", "attachments": [{"text": {"content": "Answer"}}]}, | ||
| ] | ||
| result = genie.ask_question("What is the meaning of life?") | ||
| assert result == "Answer" |
There was a problem hiding this comment.
The answer cant be that simple 🤣
aravind-segu
left a comment
There was a problem hiding this comment.
Looking at the databricks-sdk code, the get api is exactly similar if we can use it directly. But it would be half direct api code and half databricks sdk code. Upto you on if you think that would be cleaner.
Good find. Let's do the refactor in one shot to keep it readable. |
…re catches Review feedback from Ann (comments #3-7): - Remove unused pytestmark from conftest.py (#3) - Add multi-server test: UC + VS in DatabricksMultiServerMCPClient (#4) - Narrow conftest fixture catches: ExceptionGroup with McpError NOT_FOUND check instead of bare Exception — re-raises non-NOT_FOUND errors (#6) - Revert ty/type-ignore changes to non-test files (chat_models.py, lakebase.py, checkpoint.py) — not in scope for this PR (#1, #2) Error path tests across all 3 layers (#7): - Core: bad function (McpError BAD_REQUEST not found), bad tool name (McpError BAD_REQUEST malformed), wrong args (McpError missing parameter) - OpenAI Toolkit: bad function (ValueError wrapping), bad tool (ExceptionGroup), wrong args (ExceptionGroup) - OpenAI Agents: bad function (McpError), bad tool (McpError), wrong args (UserError) - LangChain: bad function (ExceptionGroup > McpError), wrong args (McpError) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…re catches Review feedback from Ann (comments #3-7): - Remove unused pytestmark from conftest.py (#3) - Add multi-server test: UC + VS in DatabricksMultiServerMCPClient (#4) - Narrow conftest fixture catches: ExceptionGroup with McpError NOT_FOUND check instead of bare Exception — re-raises non-NOT_FOUND errors (#6) - Revert ty/type-ignore changes to non-test files (chat_models.py, lakebase.py, checkpoint.py) — not in scope for this PR (#1, #2) Error path tests across all 3 layers (#7): - Core: bad function (McpError BAD_REQUEST not found), bad tool name (McpError BAD_REQUEST malformed), wrong args (McpError missing parameter) - OpenAI Toolkit: bad function (ValueError wrapping), bad tool (ExceptionGroup), wrong args (ExceptionGroup) - OpenAI Agents: bad function (McpError), bad tool (McpError), wrong args (UserError) - LangChain: bad function (ExceptionGroup > McpError), wrong args (McpError) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…re catches Review feedback from Ann (comments #3-7): - Remove unused pytestmark from conftest.py (#3) - Add multi-server test: UC + VS in DatabricksMultiServerMCPClient (#4) - Narrow conftest fixture catches: ExceptionGroup with McpError NOT_FOUND check instead of bare Exception — re-raises non-NOT_FOUND errors (#6) - Revert ty/type-ignore changes to non-test files (chat_models.py, lakebase.py, checkpoint.py) — not in scope for this PR (#1, #2) Error path tests across all 3 layers (#7): - Core: bad function (McpError BAD_REQUEST not found), bad tool name (McpError BAD_REQUEST malformed), wrong args (McpError missing parameter) - OpenAI Toolkit: bad function (ValueError wrapping), bad tool (ExceptionGroup), wrong args (ExceptionGroup) - OpenAI Agents: bad function (McpError), bad tool (McpError), wrong args (UserError) - LangChain: bad function (ExceptionGroup > McpError), wrong args (McpError) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…re catches Review feedback from Ann (comments #3-7): - Remove unused pytestmark from conftest.py (#3) - Add multi-server test: UC + VS in DatabricksMultiServerMCPClient (#4) - Narrow conftest fixture catches: ExceptionGroup with McpError NOT_FOUND check instead of bare Exception — re-raises non-NOT_FOUND errors (#6) - Revert ty/type-ignore changes to non-test files (chat_models.py, lakebase.py, checkpoint.py) — not in scope for this PR (#1, #2) Error path tests across all 3 layers (#7): - Core: bad function (McpError BAD_REQUEST not found), bad tool name (McpError BAD_REQUEST malformed), wrong args (McpError missing parameter) - OpenAI Toolkit: bad function (ValueError wrapping), bad tool (ExceptionGroup), wrong args (ExceptionGroup) - OpenAI Agents: bad function (McpError), bad tool (McpError), wrong args (UserError) - LangChain: bad function (ExceptionGroup > McpError), wrong args (McpError) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…re catches Review feedback from Ann (comments #3-7): - Remove unused pytestmark from conftest.py (#3) - Add multi-server test: UC + VS in DatabricksMultiServerMCPClient (#4) - Narrow conftest fixture catches: ExceptionGroup with McpError NOT_FOUND check instead of bare Exception — re-raises non-NOT_FOUND errors (#6) - Revert ty/type-ignore changes to non-test files (chat_models.py, lakebase.py, checkpoint.py) — not in scope for this PR (#1, #2) Error path tests across all 3 layers (#7): - Core: bad function (McpError BAD_REQUEST not found), bad tool name (McpError BAD_REQUEST malformed), wrong args (McpError missing parameter) - OpenAI Toolkit: bad function (ValueError wrapping), bad tool (ExceptionGroup), wrong args (ExceptionGroup) - OpenAI Agents: bad function (McpError), bad tool (McpError), wrong args (UserError) - LangChain: bad function (ExceptionGroup > McpError), wrong args (McpError) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…re catches Review feedback from Ann (comments #3-7): - Remove unused pytestmark from conftest.py (#3) - Add multi-server test: UC + VS in DatabricksMultiServerMCPClient (#4) - Narrow conftest fixture catches: ExceptionGroup with McpError NOT_FOUND check instead of bare Exception — re-raises non-NOT_FOUND errors (#6) - Revert ty/type-ignore changes to non-test files (chat_models.py, lakebase.py, checkpoint.py) — not in scope for this PR (#1, #2) Error path tests across all 3 layers (#7): - Core: bad function (McpError BAD_REQUEST not found), bad tool name (McpError BAD_REQUEST malformed), wrong args (McpError missing parameter) - OpenAI Toolkit: bad function (ValueError wrapping), bad tool (ExceptionGroup), wrong args (ExceptionGroup) - OpenAI Agents: bad function (McpError), bad tool (McpError), wrong args (UserError) - LangChain: bad function (ExceptionGroup > McpError), wrong args (McpError) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Per review on PR #425. Skipping #3 (trim) and #4 (conv_id naming) — addressed separately. #1 Drop owner_pod_id; ownership via heartbeat CAS on attempt_number - Remove owner_pod_id column from Response model - heartbeat_response(response_id, expected_attempt_number) CAS-checks the attempt_number column. If a heartbeat write returns 0 rows, the prior owner has been bumped by another pod's claim and the heartbeat task knows to stop. - _heartbeat() context manager takes attempt_number; passes it through from _run_background_stream / _run_background_invoke. - claim_stale_response() no longer takes a pod parameter. - _POD_LOG_ID retained for log-line identity only (not stored in DB). #2 Simplify prose recovery to json.dumps the events array - _build_prose_recovery_message: was ~110 LOC structural walker (function_call/output pairs, narrative messages, partial-text reassembly). Now ~15 LOC: filter events by prior_attempt_number, json.dumps them, wrap in a directive prompt asking the model to figure out what's done vs interrupted. #5 Drop _inject_conversation_id - The function was defensive injection of response_id into context.conversation_id when no client anchor was supplied. With rotation handling resume, and templates / chatbot consistently setting conv_id, the injection was redundant. Top-level review: proactive stale-scan loop with jitter - New _stale_response_scanner_loop: every ~30s ± 50% jitter, queries responses for in_progress rows with stale heartbeats and tries to claim+resume them. The proactive counterpart to lazy-on-GET claim; ensures crashed responses get recovered even if no client polls. - find_stale_response_ids repository function with LIMIT 50. - Spawned in the FastAPI lifespan alongside init_db; cancelled on shutdown. - Settings: stale_scan_interval_seconds=30.0, stale_scan_jitter_fraction=0.5. #6 Document /_debug/kill_task in AGENTS.md - New §4.4 explaining the test-only debug endpoint, env-var gating, what state it leaves the row in. AGENTS.md updates: - ER diagram: drop owner_pod_id, annotate attempt_number as CAS guard. - New §3.5 documenting the proactive scanner with mermaid flowchart. - §4.3 includes new scanner settings. Tests: 110 pass. Ruff/format/ty all clean. Co-authored-by: Isaac
Create the core Genie API wrapper. The wrapper API follows the Genie convseration APIs for polling.
Unit tested.