fix(workflow-operator): send provider-specific model id on HF text-gen chat fallback routes - #6972
fix(workflow-operator): send provider-specific model id on HF text-gen chat fallback routes#6972PG1204 wants to merge 6 commits into
Conversation
…-inference route in HF text-gen chat fallback
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6972 +/- ##
============================================
- Coverage 78.98% 78.52% -0.46%
- Complexity 3785 3786 +1
============================================
Files 1160 1160
Lines 46105 46031 -74
Branches 5115 5110 -5
============================================
- Hits 36414 36148 -266
- Misses 8069 8257 +188
- Partials 1622 1626 +4
*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:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 397 | 0.242 | 25,799/33,711/33,711 us | 🟢 -39.6% / 🔴 +113.3% |
| 🔴 | bs=100 sw=10 sl=64 | 928 | 0.566 | 105,438/160,379/160,379 us | 🔴 +15.9% / 🔴 +49.4% |
| 🔴 | bs=1000 sw=10 sl=64 | 1,072 | 0.654 | 924,898/1,100,901/1,100,901 us | 🔴 +15.8% / 🔴 +7.1% |
Baseline details
Latest main 06f8e42 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 397 tuples/sec | 382 tuples/sec | 787.55 tuples/sec | +3.9% | -49.6% |
| bs=10 sw=10 sl=64 | MB/s | 0.242 MB/s | 0.233 MB/s | 0.481 MB/s | +3.9% | -49.7% |
| bs=10 sw=10 sl=64 | p50 | 25,799 us | 24,514 us | 12,255 us | +5.2% | +110.5% |
| bs=10 sw=10 sl=64 | p95 | 33,711 us | 55,830 us | 15,802 us | -39.6% | +113.3% |
| bs=10 sw=10 sl=64 | p99 | 33,711 us | 55,830 us | 19,008 us | -39.6% | +77.4% |
| bs=100 sw=10 sl=64 | throughput | 928 tuples/sec | 946 tuples/sec | 997.81 tuples/sec | -1.9% | -7.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.566 MB/s | 0.577 MB/s | 0.609 MB/s | -1.9% | -7.1% |
| bs=100 sw=10 sl=64 | p50 | 105,438 us | 103,071 us | 100,690 us | +2.3% | +4.7% |
| bs=100 sw=10 sl=64 | p95 | 160,379 us | 138,428 us | 107,316 us | +15.9% | +49.4% |
| bs=100 sw=10 sl=64 | p99 | 160,379 us | 138,428 us | 113,823 us | +15.9% | +40.9% |
| bs=1000 sw=10 sl=64 | throughput | 1,072 tuples/sec | 1,100 tuples/sec | 1,030 tuples/sec | -2.5% | +4.0% |
| bs=1000 sw=10 sl=64 | MB/s | 0.654 MB/s | 0.671 MB/s | 0.629 MB/s | -2.5% | +4.0% |
| bs=1000 sw=10 sl=64 | p50 | 924,898 us | 913,947 us | 981,213 us | +1.2% | -5.7% |
| bs=1000 sw=10 sl=64 | p95 | 1,100,901 us | 950,611 us | 1,027,605 us | +15.8% | +7.1% |
| bs=1000 sw=10 sl=64 | p99 | 1,100,901 us | 950,611 us | 1,055,466 us | +15.8% | +4.3% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,503.56,200,128000,397,0.242,25798.67,33711.04,33711.04
1,100,10,64,20,2155.39,2000,1280000,928,0.566,105437.88,160378.97,160378.97
2,1000,10,64,20,18664.22,20000,12800000,1072,0.654,924897.74,1100900.71,1100900.71
|
@carloea2 I think the need to programmatically test operators becomes higher... There are more and more efforts on editing operators logic. It will be good to have the tests in CI soon. |
|
I am working on it, I will raise a discussion about it soon. |
|
/request-review @xuang7 |
|
@xuang7 github merge bot removed the pr from merge queue, requesting to add this pr to merge queue again. Thanks! |
| | if provider_name == "hf-inference": | ||
| | # hf-inference expects the model in the URL path, like the | ||
| | # pipeline route below. | ||
| | url = f"https://router.huggingface.co/hf-inference/models/{self.MODEL_ID}/v1/chat/completions" | ||
| | else: | ||
| | route = self.CHAT_ROUTES.get(provider_name, "v1/chat/completions") | ||
| | url = f"https://router.huggingface.co/{provider_name}/{route}" | ||
| | # Provider-scoped routes need the provider's own model name | ||
| | # (providerId), not the HF Hub ID. Copy instead of mutating: | ||
| | # pipeline_payload is reused for the next provider attempt. | ||
| | chat_payload = {**pipeline_payload, "model": provider_id} | ||
| | resp = requests.post(url, headers=json_headers, json=chat_payload, timeout=120) |
There was a problem hiding this comment.
might be a todo for future PR: this is python code gen base class, should this hugging face specific logic moved to somewhere else?
There was a problem hiding this comment.
Good point, so to clarify the current state: despite the Base in the name, this isn't a shared/cross-operator base class. It lives in ...operator.huggingFace.codegen and is referenced only within that package. The Base refers to it being the shared base of the HF per-task codegens, as it holds the common template and provider-fallback scaffolding that all of the TaskCodegen implementations (TextGenCodegen, ImageTaskCodegen, AudioTaskCodegen, MediaGenCodegen, QaRankingCodegen) plug their per-task payload/parse snippets into. So the Hugging Face-specific logic is already scoped to the HF package.
You're right that the name reads generic, though. I'd rather keep this PR focused on the model-id fix, but i can do a naming-clarity follow-up: e.g. renaming to HuggingFaceCodegenBase (or HuggingFacePythonTemplate) so its scope is obvious at a glance. This can be tracked as a separate follow up task.
5 participants
What changes were proposed in this PR?
Fixes two related bugs in the HuggingFace inference operator's provider-fallback loop (
_post_with_fallbackin the generated Python, emitted byPythonCodegenBase.scala) that break thetext-generation/image-text-to-textpath, which is the operator's default task.Wrong model id on provider-scoped chat routes. The chat branch posted the same payload, carrying
"model": self.MODEL_ID(the HF Hub ID), to every provider's route (router.huggingface.co/{provider}/...). Provider-scoped routes require the provider's own model name (providerId, e.g. the Hub'sQwen/Qwen2.5-72B-Instructis a different string on Groq or Fireworks), which_resolve_providersalready fetches and_call_provideralready uses for the other task families. Providers whose internal name differs from the Hub ID rejected every request with 400/404, so fallback only succeeded when the names happened to match. The chat branch now posts a per-attempt copy with the model overridden:chat_payload = {**pipeline_payload, "model": provider_id}
(a copy, not in-place mutation, pipeline_payload is reused for the next provider
attempt).
Malformed hf-inference chat URL. The chat URL for hf-inference was built as
hf-inference/v1/chat/completions, but hf-inference expects the model in the URL path:hf-inference/models/{model-id}/v1/chat/completions(the form this file already uses for pipeline tasks). Since hf-inference sorts first inPROVIDER_COST_PRIORITY, the cheapest provider failed on every row, wasting a doomed request (up to the 120 s timeout) per row before falling through.Net effect: text-gen rows now succeed on the cheapest live provider instead of failing with "All inference providers failed" or silently drifting to pricier providers. No behavior change for other task families -
_call_provideralready handled them correctly.Any related issues, documentation, discussions?
Closes #6965
How was this PR tested?
Added a generated-code test to HuggingFaceInferenceOpDescSpec ("send the provider-specific model id on provider-scoped chat routes") pinning both fixes: the corrected hf-inference chat URL, and the chat branch posting the per-provider payload copy (asserted as an anchored two-line block so pipeline routes that legitimately post pipeline_payload directly stay unaffected).
Run:
sbt "WorkflowOperator/testOnly org.apache.texera.amber.operator.huggingFace.*"
Full HF package passes (123 tests, 12 suites), including PythonCodeRawInvalidTextSpec, which py_compiles the generated Python of all 117 Python operators, verifying the edited template still emits syntactically valid Python. scalafmt clean.
Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude Fable 5 in compliance with ASF.