Repository navigation
Conversation
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com> Assisted-by: IBM Bob
|
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... |
There was a problem hiding this comment.
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.
| backend = _make_backend_with_adapter(_SIMPLE_CONFIG, adapter_models={}) | ||
| action = ALoraRequirement("The response is correct.", "uncertainty") | ||
| action = Requirement("The response is correct.") |
There was a problem hiding this comment.
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:
| 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.
| f"doesn't have the specified adapter added {adapter_name}; " | ||
| f"defaulting to regular generation" | ||
| if isinstance(action, ALoraRequirement): | ||
| raise ValueError( |
There was a problem hiding this comment.
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:
- The
exceptaroundresolve_adapter()(lines 986-993) catches the error and logs "defaulting to regular generation". For anALoraRequirementthat no longer happens. - 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 bothNone.
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(): |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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-387still says "The LLMaJ fallback in ALoraRequirement still covers only generation errors", which no longer applies.- The
Raises:section ofRequirement.validate(mellea/core/requirement.py:396-398) lists onlyAssertionError, though anALoraRequirementcan now raiseValueErrorout of it.
Pull Request
Issue
Closes #1703
Description
An explicit
ALoraRequirementwas silently downgraded to LLMaJ when its adapter couldn't be found/used. Now, anALoraRequirementraisesValueErrorwhen its adapter is unavailable:adapter_types, unresolvable, or missing anOllama model tag
AloraActivationError)This PR also updates docs/docstrings that mention the previous behaviour.
Testing
Attribution