Skip to content

fix: route compress_prompt to llmlingua2 path when use_slingua=True (#235)#242

Open
ousamabenyounes wants to merge 1 commit into
microsoft:mainfrom
ousamabenyounes:fix/issue-235
Open

fix: route compress_prompt to llmlingua2 path when use_slingua=True (#235)#242
ousamabenyounes wants to merge 1 commit into
microsoft:mainfrom
ousamabenyounes:fix/issue-235

Conversation

@ousamabenyounes
Copy link
Copy Markdown

@ousamabenyounes ousamabenyounes commented Apr 11, 2026

What does this PR do?

Fixes #235

When PromptCompressor is created with use_slingua=True, calling compress_prompt() raised:

TypeError: XLMRobertaForTokenClassification.forward() got an unexpected keyword argument 'past_key_values'

Root cause: compress_prompt() only routed to compress_prompt_llmlingua2() when self.use_llmlingua2 was True. __init__ already had the correct if use_llmlingua2 or use_slingua: guard (line 92) to call init_llmlingua2, but the dispatch in compress_prompt was missing the or self.use_slingua half.

Fix: one-line change at line 536 of llmlingua/prompt_compressor.py:

# Before
if self.use_llmlingua2:
# After
if self.use_llmlingua2 or self.use_slingua:

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Was this discussed/approved via a Github issue? Please add a link to it if that's the case.
    Issue: [Bug]: When calling slingua through PromptCompressor, use_llmlingua2 should also be set to true. #235
  • Did you make sure to update the documentation with your changes? (not applicable — no new API)
  • Did you write any new necessary tests?
    Added tests/test_slingua_routing.py with 3 unit tests covering the fixed routing, existing llmlingua2 routing, and the unchanged neither-flag path. Tests use sys.modules stubs so they run without ML dependencies.

Who can review?

@iofu728 @QianhuiWu @XufangLuo @mydmdm

Generated by Ora Studio
Vibe coded by ousamabenyounes


Vibe Coded by Ousama Ben Younes
Developed With Ora Studio (Claude Code)

…icrosoft#235)

When use_slingua=True, __init__ already called init_llmlingua2 correctly,
but compress_prompt only checked self.use_llmlingua2 and fell through to
the LLMLingua-1 causal-LM path, which passes past_key_values to
XLMRobertaForTokenClassification and crashes.

Generated by Claude Code
Vibe coded by ousamabenyounes

Co-Authored-By: Claude <noreply@anthropic.com>
@ousamabenyounes
Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Copy link
Copy Markdown
Author

@ousamabenyounes ousamabenyounes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-review (no human reviews yet)

Summary: This PR fixes a one-line routing bug: compress_prompt was not delegating to compress_prompt_llmlingua2 when use_slingua=True, even though the slingua path requires the LLMLingua-2 compression pipeline. The change is minimal and correct. A new test file exercises the three routing states (use_slingua, use_llmlingua2, neither) using sys.modules patching to avoid heavy ML dependency installation. Overall risk is low; the production change is trivial and well-motivated.

Verdict: comment

Findings (3):

  • [duplication/warning] tests/test_slingua_routing.py:62 — Near-identical test bodies for use_slingua and use_llmlingua2
    test_use_slingua_routes_to_llmlingua2 (line 62) and test_use_llmlingua2_routes_to_llmlingua2 (line 75) share an identical 10-line body differing only in the flag passed to _make_compressor. These should be collapsed into a single parameterised test (e.g. @unittest.parameterized or subTest) so future changes to the assertion pattern don't require two synchronised edits.
    confidence: 0.85

  • [bugs/warning] tests/test_slingua_routing.py:93 — Bare except swallows errors that occur before the routing branch
    test_neither_flag_does_not_route_to_llmlingua2 wraps the entire compress_prompt call in except Exception: pass. If an AttributeError or similar fires before the routing condition is even evaluated (e.g. a missing attribute introduced by a future refactor), the exception is silently discarded and the test still passes, giving a false green. Limit the swallowed exceptions to the expected stub-related ones (e.g. AttributeError, TypeError) or assert on the exception type with assertRaises.
    confidence: 0.80

  • [tests/info] tests/test_slingua_routing.py:17 — sys.modules patching relies on undocumented pytest --dist=loadfile assumption
    _install_mock_modules is called at import time and mutates the global sys.modules dict. The docstring says isolation is guaranteed by pytest --dist=loadfile, but there is no Makefile or pytest.ini in this diff enforcing that flag. If tests run in the default in-process mode, MagicMock stubs for torch/transformers/numpy could bleed into other test modules imported afterward, silently breaking integration tests. Add a conftest.py fixture or a pytest.ini marker to enforce the worker-isolation assumption, or move _install_mock_modules inside a setUp/tearDown pair with proper cleanup.
    confidence: 0.72


Generated by review-all-pr self-review pass. The fixer will attempt to address these in the same run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: When calling slingua through PromptCompressor, use_llmlingua2 should also be set to true.

1 participant