Repository navigation
chore: remove deprecated adapter shims - #1700
AngeloDanducci wants to merge 1 commit into
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
AngeloDanducci
changed the title
planetf1
left a comment
planetf1
left a comment
There was a problem hiding this comment.
Shim removal itself is clean — traced the dispatch paths across all three backends, no regressions found. One thing before merge, flagged inline. Also curious whether #1621 wants a dedicated migration note or if the auto-generated changelog covers it — not blocking.
| return f"_MutatingName({self._prefix!r})" | ||
|
|
||
|
|
||
| def test_find_adapter_survives_concurrent_removal_during_iteration(): |
There was a problem hiding this comment.
This file mixed two things: tests of the shim classes' own behaviour (fine to delete), and tests that used the shims only as convenient fixtures to exercise resolve_adapter/_find_adapter's locking and race-safety — things like the reentrant-activation-lock case, the "loser never double-registers" race, and this dict-mutated-during-iteration guard. That production code (the lock-order and snapshot-before-iterate logic in adapter.py) is untouched by this PR, but with the file gone, nothing in the test tree exercises those paths anymore — checked and there's no equivalent coverage elsewhere.
Can you port the non-shim-specific ones onto LocalFileBinding/composed-Adapter fixtures, same pattern you used for the generation-scope tests in test_huggingface_unit.py? Otherwise a future touch to that locking code has nothing to catch a regression.
| # thread's | ||
| # `add_adapter`/`resolve_adapter` — both `add_adapter`'s | ||
| # read-then-write across `_added_adapters` and `_discover_embedded_adapters`' | ||
| # mutation of global `warnings` filter state need the same lock |
There was a problem hiding this comment.
Stale comment: this still lists "_discover_embedded_adapters's mutation of global warnings filter state" as a reason for the lock. That reason no longer applies — the warnings.catch_warnings() call it refers to only existed to silence the shims' DeprecationWarning, and it's gone along with them.
The lock itself is still correct (it also guards _added_adapters's read-then-write race), so no behaviour change needed, just drop that one clause. You already made this exact edit in the equivalent huggingface.py comment — this is the one spot it didn't carry over to.
There was a problem hiding this comment.
Three failures:
FAILED test/backends/test_huggingface.py::test_adapters - KeyError: 'requirement-check_alora'
FAILED test/backends/test_huggingface.py::test_constraint_lora_with_requirement - AssertionError: assert 'requirement_check' in 'The answer should mention that there is a b in the middle of one of the strings but not the other.'
FAILED test/backends/test_huggingface.py::test_constraint_lora_override_does_not_override_alora - AssertionError: assert 'requirement_check' in 'None'
| # --------------------------------------------------------------------------- | ||
| # AdapterMixin stub methods | ||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
Do these AdapterMixin tests need to get kept? They look like they aren't quite the shim tests described by the file?
| provided. | ||
| intrinsic_name (str): Adapter function name from the index entry | ||
| (e.g. `"answerability"`). | ||
| config (dict): Parsed `io.yaml` transformation configuration. |
There was a problem hiding this comment.
We may have previously missed this; config isn't used in this function.
This branch has not been deployed
3 participants
Pull Request
Issue
Fixes #1621
Description
Removes deprecated adapter shims
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.