Skip to content

fix(adapters): raise instead of downgrading ALoraRequirement - #1733

Open
rafvasq wants to merge 1 commit into
generative-computing:mainfrom
rafvasq:fix-alora-downgrade
Open

rafvasq wants to merge 1 commit into
generative-computing:mainfrom
rafvasq:fix-alora-downgrade

Conversation

@rafvasq

@rafvasq rafvasq commented Oct 13, 2026 •

Copy link
Copy Markdown

Pull Request

Issue

Closes #1703

Description

An explicit ALoraRequirement was silently downgraded to LLMaJ when its adapter couldn't be found/used. Now, an ALoraRequirement raises ValueError when its adapter is unavailable:

  • HF, OpenAI, Ollama: adapter not registered
  • Ollama: adapter excluded by adapter_types, unresolvable, or missing an
    Ollama model tag
  • HF: adapter fails to activate (AloraActivationError)

This PR also updates docs/docstrings that mention the previous behaviour.

Testing

  • Tests added to the respective file if code was changed
  • New code has 100% coverage if code was added
  • Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

  • AI coding assistants used

Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Assisted-by: IBM Bob
@rafvasq
rafvasq requested a review from a team as a code owner October 13, 2026 18:23
@github-actions github-actions Bot added the bug Something isn't working label Oct 13, 2026
@rafvasq rafvasq changed the title fix(adapters): raise instead of downgrading alorarequirement fix(adapters): raise instead of downgrading ALoraRequirement Oct 13, 2026
@planetf1

Copy link
Copy Markdown
Contributor

From a quick look this pr looks good - it addresses the problem and isn't tangled up with some of the outstanding adapter items (they can be sorted later). Just running a more detailed code-level review...

@planetf1 planetf1 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall this looks good, apart from the inline comments.

On #1703: this covers item 1 (an explicit ALoraRequirement no longer gets downgraded), but item 3 ("make sure requirement upgrading to alora checks works correctly") is still broken. It's a behaviour bug, in code this PR doesn't touch.

"Upgrading" is when a plain Requirement is checked by the requirement-check aLoRA instead of LLM-as-a-judge, which happens by default whenever that adapter is registered. The adapter answers with a score in JSON, like {"requirement_check": {"score": 0.98}}. But Requirement.validate() (mellea/core/requirement.py:433) still reads that answer with the plain Requirement's parser, default_output_to_bool, which looks for the word "yes". There isn't one, so the check fails whatever the score. ALoraRequirement works because it carries its own JSON parser.

I reproduced this with granite-4.1-3b and the pinned adapter: a plain Requirement scored 0.977 and still came back False, while ALoraRequirement and LLMaJRequirement both returned True on the same input. It went unnoticed because the end-to-end test for this path (test/backends/test_huggingface.py:131) checks the reason text but never the pass/fail result.

It was broken before your PR, so it's your call whether to fix it here or in a follow-up. To fix it here: every backend returns the requirement it actually ran as the second-to-last item in the context it hands back (val_ctx.previous_node.node_data), so validate() can read the answer with that requirement's output_to_bool, and a rerouted plain Requirement picks up the JSON parser. I tried that locally and the existing unit tests still pass. A test that checks bool(result) follows the score for a rerouted plain Requirement would cover it. If you'd rather leave it, could you open a follow-up issue for item 3? Then "Closes #1703" can stay as it is.

Comment on lines 533 to +534
backend = _make_backend_with_adapter(_SIMPLE_CONFIG, adapter_models={})
action = ALoraRequirement("The response is correct.", "uncertainty")
action = Requirement("The response is correct.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test no longer reaches the code it's meant to cover.

A plain Requirement always looks for an adapter named requirement-check, but _make_backend_with_adapter only registers one named uncertainty. So the lookup finds nothing, the call falls back through the ordinary "no adapter" path, and the "registered but no model tag" branch (ollama.py:1005-1021) never runs. The test still passes, just for a different reason than its docstring gives.

That leaves the plain-Requirement side of that branch untested. I checked by making the branch raise: this test still passes, while the version on main fails.

Registering the adapter under requirement-check fixes it:

Suggested change
backend = _make_backend_with_adapter(_SIMPLE_CONFIG, adapter_models={})
action = ALoraRequirement("The response is correct.", "uncertainty")
action = Requirement("The response is correct.")
backend = _make_backend(adapter_models={})
backend.add_adapter(
Adapter(
identity=Identity(name="requirement-check", adapter_type="alora"),
io_contract=get_io_contract("requirement-check"),
weights=ServerMediatedBinding(),
),
config=_SIMPLE_CONFIG,
)
action = Requirement("The response is correct.")

With this, the test fails if that branch raises or drops its alora_req_adapter = None reset, and passes against the code as it is.

Comment thread mellea/backends/ollama.py
f"doesn't have the specified adapter added {adapter_name}; "
f"defaulting to regular generation"
if isinstance(action, ALoraRequirement):
raise ValueError(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When Ollama has to fetch the adapter and the fetch fails (a network error, say, or the Hub being down), an explicit ALoraRequirement ends up here with a misleading error.

What happens now:

  1. The except around resolve_adapter() (lines 986-993) catches the error and logs "defaulting to regular generation". For an ALoraRequirement that no longer happens.
  2. Execution carries on to this raise, which says no adapter is registered and suggests add_adapter(). The real cause isn't attached: __cause__ and __context__ are both None.

So the user gets advice that won't help, and the actual reason only shows up in a log line that contradicts what happened.

One fix is to raise from inside the except for an ALoraRequirement. Those lines are outside the diff, so here it is as a plain block rather than a suggestion:

except Exception as e:
    if isinstance(action, ALoraRequirement):
        raise ValueError(
            f"cannot validate an ALoraRequirement: the "
            f"{adapter_name!r} adapter could not be resolved: {e}"
        ) from e
    MelleaLogger.get_logger().warning(...)

test_alora_requirement_raises_when_resolve_fails would then match "could not be resolved", and can check isinstance(excinfo.value.__cause__, RuntimeError). I ran that combination: the whole Ollama test file passes, and the updated test fails against the current code.

Also, on Ollama add_adapter() on its own isn't enough. The adapter needs a model tag in adapter_models as well, otherwise the next attempt fails with "no Ollama model tag". It'd help if this message mentioned adapter_models.

await ALoraRequirement("must mention Paris").validate(backend, ctx)


async def test_alora_requirement_raises_when_adapter_not_registered():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This covers the new raise for ALoraRequirement. What nothing in the OpenAI or HF unit tests checks is the other side: that a plain Requirement with no adapter registered still falls back to LLM-as-a-judge.

That matters because the raise and the fallback are now one isinstance check apart (here at openai.py:929, and huggingface.py:886). I changed both checks to raise for every requirement and ran the backend and stdlib unit suites, and nothing failed. If that check ever broke, every plain Requirement on these backends would start raising, since most users never register the adapter.

The Ollama file already does this well: test_alora_requirement_raises_when_adapter_never_registered tests both cases side by side. A matching pair for OpenAI and HF would close the gap.

# prompt. Only an auto-rerouted plain Requirement may
# fall back to LLM-as-a-judge.
if isinstance(action, ALoraRequirement):
raise ValueError(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: two small points on this raise.

First, the AloraActivationError docstring (adapters/_core.py:112-119) still says Requirement.validate() catches this error and falls back to LLM-as-a-judge. That's now only true for a plain Requirement, so a clause saying an explicit ALoraRequirement raises ValueError would bring it in line. It's the one description of the old behaviour this PR doesn't update.

Second, wrapping the error in a plain ValueError means callers can't catch it with except AloraActivationError on this path, even though that class is already a ValueError subclass. The useful details (which adapter, the base model, the missing invocation tokens) also end up only on __cause__. Re-raising the original, or putting {exc} in this message, would keep them visible and still satisfy "raises ValueError".

> LoRA adapter for validation instead of LLM-as-a-judge. It falls back to LLM-as-a-judge
> if the adapter is unavailable. See [LoRA and aLoRA Adapters](../advanced/lora-and-alora-adapters.md).
> LoRA adapter for validation instead of LLM-as-a-judge. If the adapter is unavailable,
> validation raises `ValueError` rather than falling back — use a plain `Requirement` if

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: this says validation raises ValueError, which is true for HF, OpenAI and Ollama. On LiteLLM and watsonx an ALoraRequirement raises a bare Exception ("does not support aLoRA adapters"), so an except ValueError written from this page won't catch it there. The ALoraRequirement docstring (requirement.py:68-69) says the same. Naming the three backends here would make it accurate; changing the exception type on the other two could be a follow-up issue.

error is surfaced on the returned `ValidationResult.error` by
`Requirement.validate` — it is not caught here and does not trigger the
LLMaJ fallback, which covers only generation errors. The result fails
`Requirement.validate`. The result fails

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: a few docs leftovers:

  • Lines 75-76: the sentence got shorter but wasn't rewrapped, so "The result fails" breaks the line early.
  • test/stdlib/requirements/test_requirement.py:386-387 still says "The LLMaJ fallback in ALoraRequirement still covers only generation errors", which no longer applies.
  • The Raises: section of Requirement.validate (mellea/core/requirement.py:396-398) lists only AssertionError, though an ALoraRequirement can now raise ValueError out of it.

This branch has not been deployed

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: issues with AloraRequirement downgrading / requirement upgrading

2 participants