Repository navigation
feat(agent-service): add stateless /recommend endpoint (V1 hardcoded) - #6436
gupta-sahil01 wants to merge 4 commits into
Conversation
Automated Reviewer SuggestionsBased on the
|
|
👋 Thanks for your first contribution to Texera, @gupta-sahil01! If you're looking for a good place to start, browse issues labeled You can drive common housekeeping yourself by commenting one of these commands on its own line:
Each command must match exactly: |
|
Really clean V1 — the hardcoded/strategy-discriminator split and the catalog validation are a nice way to ship the whole pipeline at zero API cost. Two forward-looking notes, both about what the endpoint
Neither blocks V1 — just wanted to surface them while the request shape is still easy to evolve. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6436 +/- ##
============================================
+ Coverage 70.57% 70.67% +0.10%
Complexity 3425 3425
============================================
Files 1146 1149 +3
Lines 45062 45235 +173
Branches 4960 4960
============================================
+ Hits 31802 31972 +170
- Misses 11613 11616 +3
Partials 1647 1647
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@gupta-sahil01 Can you change the word "ghost" to a better name? |
Yes professor, will do |
Ambient operator recommender, Version 1 (apache#5240). Adds a stateless POST /api/recommend that, given the operator a user just added, returns a short ranked list of likely next operators for the canvas to render as ghost suggestions. V1 ranks from a hardcoded rule table (no LLM call, no persistent state), so it ships and validates the full pipeline at zero API cost, per the discussion's V1/V2 split. Suggestions are validated against the live operator catalog (WorkflowSystemMetadata) when available, so a stale rule degrades to "not suggested" rather than a broken ghost. The response carries a `strategy` discriminator ("hardcoded") so V2 can swap in a small LLM behind the same request/response shape without a breaking change.
bda9360 to
b95928d
Compare
xuang7
left a comment
xuang7
left a comment
There was a problem hiding this comment.
Thanks for the PR. Generally LGTM. For the hardcoded rules, consider adding RATIONALE entries for the operators that currently fall back to the generic reason. Also, please resolve the Copilot comments where helpful.
|
Thanks for addressing the comments. I saw that Copilot left a few comments. Please address them if they are helpful. |
…dling - Reject whitespace-only operatorType (schema now requires a non-whitespace character) instead of returning 200 with an empty list; add a regression test. - Return a generic message on 500 responses (the real error is still logged) to avoid leaking internal details. - Relax the OperatorRecommendation docstring to match actual behavior (suggestions are validated against the catalog only when it is available).
|
This pull request has been automatically marked as stale because it has If this PR is still relevant, please:
To exempt this PR from auto-close, a maintainer can add the |
|
/request-review @xuang7 |
The head was stale, and so is `base.sha`: it is main's tip at the pull request's last synchronize, not now. For any PR nobody has pushed to since apache#6941 landed, it predates release-branches.yml and its parser, so the `run:` step exits 2 under `bash -eo pipefail` and this required check goes red with a file-not-found that no approval or unlabel can clear. Measured: 33 of the 221 open pull requests into main have such a base — apache#6009's is dated 2026-07-10, apache#6436's 2026-07-18, both still active today. This is not a prediction. backport-auto-label.yml pins the same ref and has been failing exactly this way in production for a month (run 31542186218, 2026-08-11), survivable only because that check is not required. Read the default branch instead, which always has the config and is what Direct Backport Push already reads — so both consumers judge a backport by the same file. It also means a PR retargeted onto a release branch, where the config does not exist, reaches the report instead of dying before it. Also corrects the header comment: since the context moved into its own `~DEFAULT_BRANCH`-only ruleset, a PR into a release branch is no longer waiting on it. The reason not to condition on the base branch is simply that a backport is not itself backported onward.
|
Any updates here? |
|
@Xiao-zhen-Liu can you help on review this PR? it is a good feature |
Xiao-zhen-Liu
left a comment
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
Thanks for putting this together! Starting with a deterministic V1 rule table and live catalog validation is a clean way to validate canvas suggestions without inference cost.
Across the PR, the endpoint path, module folder, filenames, and request and response types all claim generic recommendation names, whereas this feature only suggests next operators. Because Texera and agent-service may later recommend datasets, workflows, links, or operator settings, could we scope the route, directory, files, and types specifically to operator recommendation (matching #6437)? Merging main will also trigger CI, which did not run on the latest commit.
| // Never suggest the operator itself, and never suggest the same type twice. | ||
| if (candidate === operatorType || seen.has(candidate)) continue; | ||
| // Drop suggestions the running system can't actually instantiate. | ||
| if (catalogReady && !catalog!.operatorTypeExists(candidate)) continue; |
There was a problem hiding this comment.
Main has removed this existence helper from the operator catalog (#9239). Once merged with main, typechecking fails and every request returns 500 after startup loads the catalog (for example, requesting suggestions after adding CSV File Scan throws because the method is missing). Could we merge main and check existence through what the catalog still exposes, such as looking up the operator's schema?
| // hardcoded rule table with no LLM call, so it needs no user token and no | ||
| // per-request model configuration; it only reads the process-wide operator | ||
| // catalog to validate and label its suggestions. | ||
| const recommendRouter = new Elysia({ prefix: "/recommend" }) |
There was a problem hiding this comment.
On its own, this PR leaves the new endpoint unreachable from the browser: the frontend dev proxy, single-node web server config, and Kubernetes gateway only forward the existing agents route to the agent service (#6437 adds the first two, while the gateway route was dropped in a merge conflict). Could we wire all three routes here, using a separate Kubernetes route so stateless calls bypass workflow-header consistent hashing?
| // Stateless ambient operator recommender (apache/texera#5240). Given the | ||
| // operator a user just added, returns a short ranked list of likely next | ||
| // operators for the canvas to render as suggestions. Version 1 uses a | ||
| // hardcoded rule table with no LLM call, so it needs no user token and no |
There was a problem hiding this comment.
Leaving this endpoint unauthenticated will force a breaking change in V2: the agent service holds no LLM key of its own and forwards the signed-in user's Bearer token to the LLM gateway (#5421), while the Kubernetes gateway does not enforce auth on agent-service routes. Because the workflow editor already attaches the user's Bearer token on HTTP requests, could we validate it here just like agent creation does so V2 needs no auth contract change?
| set.status = 400; | ||
| return { error: "operatorType must be a non-empty string" }; | ||
| } | ||
| return recommendOperators({ ...req, operatorType }, WorkflowSystemMetadata.getInstance()); |
There was a problem hiding this comment.
If the agent service starts before the backend is ready, startup logs a warning and leaves the catalog uninitialized. Chat agents retry loading the catalog on creation, whereas this handler keeps reading the uninitialized catalog on every request, returning unvalidated operator types with no display names until someone creates a chat agent. Could we lazily retry initializing the catalog here when it is not ready yet?
| * The operator type that was just added and whose output port we are | ||
| * suggesting successors for (e.g. `"CSVFileScan"`). Primary ranking signal. | ||
| */ | ||
| operatorType: string; |
There was a problem hiding this comment.
Neither the request nor each returned suggestion identifies a port, which forces the canvas to hardcode the first output port and first input port. That breaks multi-output operators like Split and multi-input targets like Sklearn Prediction, whose first input port expects a trained model while its second expects data rows. Could we include the source output port in the request and the target input port on each suggestion so multi-port operators work without a future contract break?
| import type { RecommendationResponse } from "./recommender/recommender-types"; | ||
|
|
||
| const API = env.API_PREFIX; | ||
| const app = buildApp(); |
There was a problem hiding this comment.
These route tests never initialize the real operator catalog, so catalog validation and display-name lookup are only exercised against the unit-test stub (which hid the removed catalog method on main). Could we add a route test with an initialized catalog fixture so mismatches with the real catalog class are caught?
| return { error: errorMessage || "Internal server error" }; | ||
| }) | ||
| .post( | ||
| "/", |
There was a problem hiding this comment.
Nit: Prettier fails on the 5-space indentation in this handler and in the whitespace test in the route spec. Could we run the formatter before pushing?
| PostgreSQLSource: "source", | ||
| MySQLSource: "source", | ||
| AsterixDBSource: "source", | ||
| SQLSource: "source", |
There was a problem hiding this comment.
Nit: This entry refers to an abstract base descriptor rather than a registered operator (and Parallel CSV File Scan above is disabled in the catalog). Could we keep only concrete registered operators in this table?
| /** Whether an operator is a terminal visualization/sink with no useful successor. */ | ||
| export function isSink(operatorType: string): boolean { | ||
| if (EXTRA_SINKS.has(operatorType)) return true; | ||
| if (OPERATOR_CATEGORY[operatorType] === "sink") return true; |
There was a problem hiding this comment.
Nit: None of the entries in the category map above use the sink label, so this check never matches. Could we drop it?
| * heuristic. Operators absent here fall back to {@link inferCategory}. | ||
| */ | ||
| const OPERATOR_CATEGORY: Record<string, OperatorCategory> = { | ||
| // Sources — read data into the workflow. |
There was a problem hiding this comment.
Nit: Could we replace the em-dashes in the comments across these files with colons or parentheses to match project style?
What changes were proposed in this PR?
Backend half (PR 1 of 2) of the ambient operator recommender, Version 1 — see #6293 (design discussion #5240).
Adds a stateless
POST /api/recommendendpoint toagent-service: given the operator a user just added to the canvas, it returns a short ranked list of likely next operators for the frontend to render as suggestion chips.WorkflowSystemMetadata) when available, so a stale rule degrades to "not suggested" rather than a broken suggestion.strategydiscriminator ("hardcoded"/"llm") so V2 can swap in a small-LLM ranker behind the same request/response shape without a breaking change.The frontend that consumes this endpoint (suggestion rendering, opt-in flag, click-to-materialize) follows in PR 2.
Any related issues, documentation, discussions?
How was this PR tested?
bun testinagent-service/— 131 passed, including newsrc/recommender/recommender.spec.tsandsrc/server.recommend.spec.tscovering ranking order, catalog validation, terminal sinks (no suggestions), limit clamping, and 400s on malformed/empty input. Also verified live:POST /api/recommendwith{"operatorType":"CSVFileScan"}returns the expected ranked JSON.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Anthropic Claude (Claude Code)