fix: five runtime crashes from names that were never bound - #513
Conversation
re.template was deprecated in 3.11 and removed in 3.13, so importing dataflow.prompts.text2sql raised ImportError and took every text2sql operator and pipeline with it. The name was never used.
_read_audio_base64 called base64.b64decode without the import, so any base64 audio input raised NameError.
Providers send "content": null when a reply carries only tool calls or
only reasoning. .get('content', '') returns None there, so the regex
below raised TypeError, which the caller swallowed into a dropped row
after five pointless retries.
The guard raised NameError instead of the intended ValueError, hiding which method was rejected.
…estion Both names were read but never assigned, so the 'no valid reference answers' branch raised AttributeError then NameError on an ordinary data condition. Mirrors ReasoningAnswerModelJudgeFilter, which this operator was derived from.
Each test fails on the parent commit and passes with its fix.
support_subquestions was assigned only in the semantic branch, but statistic() reads it and match mode calls statistic(), so match mode raised AttributeError. 'from email.policy import strict' was an unused editor auto-import, the same class of mistake as the re.template one. Also documents keep_all_samples in get_desc and tightens the tests to decode real base64 audio rather than assert the module attribute.
|
Thanks again for contributing these runtime fixes. Since these changes are also included in #514, I suggest using this PR as the main PR for the cross-module fixes, including the API serving, Text2SQL, LALM, code filter, and evaluator-related crash fixes. After this PR is merged, #514 can be rebased onto the latest |
|
Since this PR touches several different modules, I’d also like to invite the main contributors and maintainers of the affected files to help review the corresponding changes:
Could you please take a look at the parts related to your respective modules when convenient? Thanks! |
• dataflow/serving/api_llm_serving_request.py: Improve robustness when provider responses have null/absent message/content. Use safer message extraction, treat content==None as failure unless reasoning_content exists (in which case wrap reasoning into <think>/<answer>), and return None for pure tool-call/no-content replies to avoid silently dropped rows. • dataflow/operators/core_text/eval/bench_dataset_evaluator_question.py: Only compute subquestion statistics when support_subquestions is enabled AND compare_method is "semantic" to avoid reading semantic-only attributes in match mode. • test/cpu_only/test_undefined_name_crashes.py: Update tests to reflect new behavior: rename/rewrite the null-content test to expect None for tool-call-only responses, parametrize the bench evaluator test over support_subquestions and pass the flag into the evaluator, and remove an obsolete assertion about a template attribute.
|
Hi @SunnyHaze thanks for clarifying the preferred structure. I’ll keep #513 as the main PR for the shared runtime fixes and wait for feedback from the relevant module maintainers. It #513 is merged, i can update #514 from the latest main and retain only the evaluator refactor, its fixes, and the corresponding tests. If you would prefer to adjust #514 directly, maintainer edits are also welcome! Thanks -MB |
2 participants
Five crashes where a name is read but never bound. Each is independent and has a regression test that fails without its fix.
The
re.templateone is why I went looking: removed in Python 3.13, soimport dataflow.prompts.text2sqlraises ImportError and takes every text2sql operator with it. The rest: a missingbase64import in LALM serving, aNameErrorinCodeGenericScoreFilter's error path, and two unbound attributes inBenchDatasetEvaluatorQuestion(required_columnsandkeep_all_sampleson the empty-reference path), plussupport_subquestions, whichstatistic()reads in match mode. I mirroredReasoningAnswerModelJudgeFilter, which this operator derives from and binds both. Also droppedfrom email.policy import strict, an unused auto-import of the same kind.Verified on 3.10, 3.12 and 3.13.
pytest test/goes from 74 to 75 passing, no change to the 8 pre-existing failures. I reverted each fix in turn to confirm its test fails.Happy to adjust.