fix(HuggingFace): drop unused TensorFlow import in sentiment analysis codegen - #6809
Conversation
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6809 +/- ##
============================================
- Coverage 83.20% 80.37% -2.83%
- Complexity 4117 4691 +574
============================================
Files 1166 1222 +56
Lines 46456 50684 +4228
Branches 5177 5760 +583
============================================
+ Hits 38654 40739 +2085
- Misses 6096 8096 +2000
- Partials 1706 1849 +143
*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:
|
… codegen HuggingFaceSentimentAnalysisOpDesc.generatePythonCode imported TFAutoModelForSequenceClassification but never used it (the model loads via the PyTorch AutoModelForSequenceClassification). The TF-only symbol forces a TensorFlow backend, so in a PyTorch-only transformers install the import raises and crashes the generated script at module load. Remove the unused import. Adds a HuggingFaceSentimentAnalysisOpDescSpec regression test asserting the generated code no longer imports the TF-only symbol while still importing the PyTorch class it uses. Closes apache#6794 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c76dc6b to
b5303ed
Compare
|
@carloea2, @aglinxinyuan May any of you look at it please? Thank you. |
✅ No material benchmark regressions detected🟢 0 better · 🔴 0 worse · ⚪ 15 noise (<±5%) · 0 without baseline
Baseline detailsLatest main
Raw CSVconfig_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,444.87,200,128000,450,0.274,20708.40,30940.25,30940.25
1,100,10,64,20,2057.43,2000,1280000,972,0.593,100305.90,125127.51,125127.51
2,1000,10,64,20,17673.46,20000,12800000,1132,0.691,877808.41,929214.26,929214.26 |
There was a problem hiding this comment.
Pull request overview
This PR fixes HuggingFace Sentiment Analysis Python code generation by removing an unused TensorFlow-only transformers import that can crash the generated script in PyTorch-only environments, aligning behavior with the operator’s actual PyTorch usage (Closes #6794).
Changes:
- Remove the unused
TFAutoModelForSequenceClassificationimport from the generated Python code. - Add a regression test ensuring the TF-only symbol is not emitted while the PyTorch
AutoModelForSequenceClassificationimport remains present.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/HuggingFaceSentimentAnalysisOpDesc.scala | Drops the unused TensorFlow-only import from generatePythonCode() output. |
| common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/huggingFace/HuggingFaceSentimentAnalysisOpDescSpec.scala | Adds regression coverage verifying the generated code no longer contains the TF-only import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Please fix the ci |
The assertion only guards a line in generatePythonCode() that is slated for removal, so it adds little lasting value. Keep the import fix itself. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
CI green now — the title check was a transient failure, re-triggered by the new push. Also removed the test per @Yicong-Huang (4defe25). Thanks! |
Please don't forget to update PR description ;) |
|
Done, updated the description. |
|
Do we have a way to test and verify the generated python code can run as expected? cc @carloea2 to see if you have context on this? |
It's a separate topic, shouldn't block this PR. I think currently all Python operators are untested by Python interpreter. |
|
If there are no programmatical tests, I will ask @kz930 to manually test and report the result in PR description. Thanks. |
|
I made a programmatic test to check all python operators generated code can py_compile. However it does not run them because of 2 reasons: Increased run time. Do you think is good idea to actually run them to fully test them? |
|
Also the test is supposed to check the operator correctly uses and don't forget to use Python template builder. |
I think it will be great to have these added to CI! we can just ask each operator author/maintainer to provide a script to generate smoke data. Your test should provide an API to get data and get operator. For this PR, if we have manual test, I am fine with it. We can push the programmatic test to future work. @carloea2 do you have an issue for it? |
|
I do not have an issue, that test was merged in the next PR: #4189 Do you want an issue for also running them? |
…ed-tf-import-6794
|
Manually verified, numbers in the description: with the pinned For a programmatic version — #7207 routes tests that actually run generated code to |
Backport auto-label reportThis
|
What changes were proposed in this PR?
HuggingFaceSentimentAnalysisOpDesc.generatePythonCodeemitted an import forTFAutoModelForSequenceClassificationthat the generated body never uses — the model is loaded through the PyTorchAutoModelForSequenceClassification.The symbol does not exist in the pinned
transformers: the library dropped its TensorFlow backend in 5.0, andamber/operator-requirements.txthas been on 5.x since #5016 (5.5.0 today). So the import is not dead weight — it raisesImportErrorat module load and takes the whole generated script down before any inference runs, on every environment that installs the pinned requirements.Fix — remove the unused import:
from transformers import pipeline from transformers import AutoModelForSequenceClassification -from transformers import TFAutoModelForSequenceClassification from transformers import AutoTokenizer, AutoConfigAny related issues, documentation, discussions?
Closes #6794
How was this PR tested?
Manually, per @Yicong-Huang's request: the existing spec asserts on the emitted text and
py_compileonly parses it, so neither executes the template.The template was dumped from
generatePythonCode(columnstext→pos/neu/neg) and run against thepytexerainamber/src/main/python, in a virtualenv with the pinnedtransformers5.5.0 andnumpy2.1.0. main's stops at line 4 withImportError: cannot import name 'TFAutoModelForSequenceClassification' from 'transformers', before the class body is defined; this PR's loadscardiffnlp/twitter-roberta-base-sentiment-latestand classifies all three rows, each row's three columns summing to 1:Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)