Repository navigation
[api][yaml] Add aliases for six unaliased resource implementations - #962
Conversation
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for taking this on. Only one inline comment below.
FYI in case it's useful, since you deliberately scoped item 2 out: it has a doc-only PR up in #963, so that half is covered and this one gets to stay purely additive.
| ) | ||
| assert resolve_clazz( | ||
| "azure_openai", ResourceType.CHAT_MODEL_CONNECTION, "java" | ||
| ).endswith("AzureOpenAIChatModelConnection") |
There was a problem hiding this comment.
AzureOpenAIChatModelConnection is the simple name on both sides of the bridge:
- Python:
...chat_models.azure.azure_openai_chat_model.AzureOpenAIChatModelConnection - Java:
...integrations.chatmodels.openai.AzureOpenAIChatModelConnection
So this passes even if the java entry ever lost its .Java and resolved to the Python class, and the same goes for the Setup assertion two lines down. The Java mirror catches that case because isEqualTo compares the whole constant, so the two sides aren't guarding equally right now.
test_resolve_clazz_dispatches_on_language above already pins the package for this reason. Would it be worth borrowing that here?
azure_conn = resolve_clazz("azure_openai", ResourceType.CHAT_MODEL_CONNECTION, "java")
assert azure_conn.startswith("org.apache.flink.agents")
assert azure_conn.endswith("AzureOpenAIChatModelConnection")azure_openai is the only one of the six where the names collide, so it would just be these two.
There was a problem hiding this comment.
Good catch, thanks. ab251a7 pins the package on both the connection and setup assertions for azure_openai — startswith("org.apache.flink.agents") plus the existing endswith, mirroring test_resolve_clazz_dispatches_on_language. It's the only one of the six where the Java/Python simple names collide, so the other five keep the endswith-only form. 13 passed locally on test_aliases.py.
There was a problem hiding this comment.
Good catch — done in 78b8328. azure_openai now asserts both startswith("org.apache.flink.agents") and endswith(...) on the connection and the setup, mirroring test_resolve_clazz_dispatches_on_language. 13 passed locally, including test_resolve_clazz_covers_chat_model_java_gemini_and_azure_openai.
|
One note on the body: GitHub's closing-keyword parser stops at the number, so One other thing worth folding in: the repo keeps a PR template at |
azure_openai is the only one of the six aliases whose Java and Python
simple names collide (AzureOpenAIChatModelConnection), so endswith-only
assertions would still pass if the Java entry lost its `.Java` suffix and
resolved to the Python class. Add a startswith("org.apache.flink.agents")
guard on both the connection and setup assertions, mirroring
test_resolve_clazz_dispatches_on_language.
Review feedback on apache#962.
|
Thanks @weiqingy, both points addressed in ab251a7:
|
The YAML loader resolves short clazz aliases through three hand-maintained tables (Java Aliases, Python aliases.py, docs yaml.md). Six resource implementations have a ResourceName constant but no alias entry, so users must reach them by fully-qualified class path. Add the missing entries across all three tables: - gemini -> Gemini (Java) chat model connection + setup - azure_openai -> Azure OpenAI (Java) chat model connection + setup - opensearch -> OpenSearch (Java) vector store - s3_vectors -> S3 Vectors (Java) vector store - milvus -> Milvus (Java) vector store - mem0 -> Mem0 (Python) vector store All additions mirror the shape of existing entries (e.g. the Bedrock aliases added in apache#812) and are purely additive; the existing 'azure' alias (which resolves to the Azure AI Inference connection, not Azure OpenAI) is intentionally left untouched. Resolving what 'azure' should mean and aligning the docs row for it is item 2 of apache#958 and is left for a separate, behavior-changing PR as the issue suggests. Tests: extend AliasesTest (Java) and test_aliases.py (Python) to assert the six new aliases resolve to the expected FQNs. Python suite passes locally (13/13); Java build deferred to CI (mechanical change mirroring adjacent entries). Closes apache#958 (item 1).
Spotless/google-java-format (AOSP) flagged the two-line form of the geminiSetup assignment in AliasesTest; the call fits on a single line (100 chars), which is the form the formatter expects.
azure_openai is the only one of the six aliases whose Java and Python
simple names collide (AzureOpenAIChatModelConnection), so endswith-only
assertions would still pass if the Java entry lost its `.Java` suffix and
resolved to the Python class. Add a startswith("org.apache.flink.agents")
guard on both the connection and setup assertions, mirroring
test_resolve_clazz_dispatches_on_language.
Review feedback on apache#962.
ab251a7 to
78b8328
Compare
|
Thanks @wenjin272 and @weiqingy for the review. Conflict resolved — rebased onto @weiqingy's top-level points — both addressed:
@weiqingy's line comment on |
…t seams, docs - Python: treat whitespace-only model / api_key / api_base_url the same as absent, matching the Java connection, with tests for the ' ' cases - Java: make withVLLMDefaults package-visible and assert explicit values are preserved and defaults injected only when absent - Document that the vLLM connection deliberately does not consult the OPENAI_API_KEY / OPENAI_API_BASE_URL environment variables - Add the vLLM row to the FAQ Native Integration Support Matrix - Fix the Java parameter table types to int, matching the other tables - Merge main and cover the vllm alias in AliasesTest / test_aliases.py following the pattern established by apache#962
Linked issue: #958
Purpose of change
The YAML loader resolves short
clazz:aliases through three hand-maintained copies of the same alias table (JavaAliases.java, Pythonaliases.py, docsyaml.md). Six resource implementations already have aResourceNameconstant but no alias entry in any of the three tables, so users must reach them by fully-qualified class path. Per item 1 of #958, this adds the missing entries across all three tables, mirroring the shape of existing entries (e.g. the Bedrock aliases added in #812).geminiazure_openaiopensearchs3_vectorsmilvusmem0All additions are purely additive. Java/Python/API-YAML alignment is preserved (the same six entries land in the Java table, the Python table, and the docs table).
Scope: item 1 of #958 only. Item 2 — the
azurealias resolving to the Azure AI Inference connection (AzureAIChatModelConnection) whileyaml.mddocuments it as "Azure OpenAI (Java)" — is a behavior change for users who already haveclazz: azurein their YAML and is intentionally left for a separate PR, as the issue suggests. The existingazurealias and its docs row are untouched here. Addingazure_openaias a new Java alias is independent of that decision and gives the Azure OpenAI Java connection a reachable short name either way.Open question from the issue: should any of these six be deliberately unaliased? I defaulted to aliasing all six, since an alias is an optional short name (fully-qualified
clazz:still works) and adding one is reversible. Happy to drop any a maintainer feels should stay first-class-only.Tests
python/flink_agents/api/yaml/tests/test_aliases.py: added two cases asserting the six new aliases resolve to the expected FQNs. Forazure_openai— the only alias whose Java/Python simple names collide (AzureOpenAIChatModelConnection) — the assertions pin the package prefix (startswith("org.apache.flink.agents")) in addition to the simple name, so a Java entry that lost its.Javasuffix and resolved to the Python class would fail. Full file passes locally:13 passed.api/src/test/java/org/apache/flink/agents/api/yaml/AliasesTest.java: added two parallel cases mirroring the existingclazzAliasJavaBucketstyle.ruff checkclean on the changed Python file.Java build was not run locally (sparse checkout of a large repo; host JDK is 8 vs the project Java 11 target). The change is mechanical — each new entry mirrors the immediately adjacent existing
put/dict lines — and the referencedResourceNameconstants all exist atmain. CI will confirm.API
No public API signature changes. Aliases are optional short names; fully-qualified
clazz:values keep passing through unchanged.Documentation
doc-includeddocs/content/docs/development/yaml.mdis updated in this PR: theazure_openairow gains atype: javacolumn entry, andmem0/opensearch/s3_vectors/milvusrows are added to the vector-store alias table.Part of #958 (item 1).
Generated-by: Claude Code (Anthropic) — used to author this patch and PR description; reviewed and submitted by a human contributor.