Skip to content

FIX BdLoraConfig.nblocks default value#3252

Open
ATOM00blue wants to merge 1 commit into
huggingface:mainfrom
ATOM00blue:fix-bdlora-nblocks-default
Open

FIX BdLoraConfig.nblocks default value#3252
ATOM00blue wants to merge 1 commit into
huggingface:mainfrom
ATOM00blue:fix-bdlora-nblocks-default

Conversation

@ATOM00blue

Copy link
Copy Markdown

What this fixes

BdLoraConfig.nblocks is meant to default to the integer 1, but a misplaced trailing comma wrapped the field(...) call in a tuple:

nblocks: int = (
    field(
        default=1,
        ...
    ),
)

Because the assigned value is a tuple rather than a Field instance, the dataclass machinery treats it as a literal default. So BdLoraConfig().nblocks is the one-element tuple (Field(...),) instead of 1.

This breaks get_peft_model whenever a BD-LoRA config is used without explicitly setting nblocks, because the value flows into BlockDiagonalLinear where it is used for % and //:

class MyModule(nn.Module):
    def __init__(self):
        super().__init__()
        self.linear = nn.Linear(1000, 1000)
    def forward(self, x):
        return self.linear(x)

model = MyModule()
bdlora_config = {"target_modules_bd_a": ["linear"], "target_modules_bd_b": []}  # no nblocks -> default
config = LoraConfig(target_modules=["linear"], use_bdlora=bdlora_config)
get_peft_model(model, config)
# TypeError: unsupported operand type(s) for %: 'int' and 'tuple'

The existing BD-LoRA tests all pass an explicit nblocks, so they did not catch the broken default.

The fix

Remove the extra parentheses and trailing comma so nblocks is a regular integer field defaulting to 1, consistent with the other fields in BdLoraConfig.

Tests

Added two tests in TestLoraInitialization (both fail before the fix, pass after):

  • test_bdlora_default_nblocks_is_int checks BdLoraConfig().nblocks == 1.
  • test_bdlora_get_peft_model_with_default_nblocks builds a BD-LoRA model relying on the default nblocks and checks the resulting block-diagonal layer has nblocks == 1.
pytest tests/test_initialization.py -k bdlora
# 5 passed

ruff check and ruff format --check are clean on the changed files.


Developed with AI coding assistance; reviewed and submitted by the author.

A misplaced trailing comma wrapped the `field(...)` call for
`BdLoraConfig.nblocks` in a tuple, so the default value was a one-element
tuple containing a `Field` object instead of the integer `1`. As a result,
calling `get_peft_model` with a BD-LoRA config that did not explicitly set
`nblocks` raised `TypeError` when the value was used for modulo and
floor-division in `BlockDiagonalLinear`.

Remove the extra parentheses and trailing comma so `nblocks` defaults to
`1` as documented. Add tests that build a BD-LoRA model relying on the
default `nblocks` and check the default is the integer `1`.
Copilot AI review requested due to automatic review settings May 22, 2026 02:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes BD-LoRA configuration initialization by correcting BdLoraConfig.nblocks to default to the integer 1 (instead of an accidental 1-tuple), and adds regression tests to ensure get_peft_model works when nblocks is not explicitly provided.

Changes:

  • Fix BdLoraConfig.nblocks dataclass field definition so the default is a proper field(default=1, ...).
  • Add tests covering the default nblocks value and successful BD-LoRA model creation relying on that default.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/peft/tuners/lora/config.py Corrects BdLoraConfig.nblocks default to be an int via a proper dataclasses.field.
tests/test_initialization.py Adds regression tests asserting the default nblocks is 1 and that get_peft_model succeeds without explicitly setting it.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +219 to 225
nblocks: int = field(
default=1,
metadata={
"help": "Number of blocks each block-diagonal matrix has. If using BD-LoRA to speed up inference, "
"set it to be equal to the desired sharding degree during serving."
},
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ignore this

@BenjaminBossan BenjaminBossan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You have correctly identified an error. The fix is correct but let's test it differently.

with pytest.raises(ValueError, match="not divisible by"):
get_peft_model(model, config)

def test_bdlora_default_nblocks_is_int(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need dedicated tests for this. Our normal tests should already surface the error but they don't because they all override nblocks=2, so the default is never used:

(
"BD-LoRA A only",
"MLP",
LoraConfig,
{
"target_modules": ["lin0", "lin1"],
"use_bdlora": BdLoraConfig(target_modules_bd_a=["lin0"], nblocks=2, match_strict=False),
},
),
(
"BD-LoRA B only",
"MLP",
LoraConfig,
{
"target_modules": ["lin0", "lin1"],
"use_bdlora": BdLoraConfig(target_modules_bd_b=["lin1"], nblocks=2, match_strict=False),
},
),
(
"BD-LoRA both A and B",
"MLP",
LoraConfig,
{
"target_modules": ["lin0", "lin1"],
"use_bdlora": BdLoraConfig(target_modules_bd_a=["lin0"], target_modules_bd_b=["lin1"], nblocks=2),
},
),

So to trigger the error, we can just remove nblocks=2 from one of these settings.

Comment on lines +219 to 225
nblocks: int = field(
default=1,
metadata={
"help": "Number of blocks each block-diagonal matrix has. If using BD-LoRA to speed up inference, "
"set it to be equal to the desired sharding degree during serving."
},
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ignore this

@BenjaminBossan

Copy link
Copy Markdown
Member

@ATOM00blue Are you still on this?

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.

3 participants