Add type hints to utility functions in merge_utils and other modules - #3144
Conversation
Add missing type annotations to 8 functions across `src/peft/utils/merge_utils.py` and `src/peft/utils/other.py`: - `reshape_weight_task_tensors`: added `torch.Tensor` param/return types - `_get_input_embeddings_name`: added `torch.nn.Module`, `Optional[str]` types - `_get_submodules`: added `torch.nn.Module`, `str`, and tuple return type - `_get_submodules_with_grandparent`: added full param/return types - `_freeze_adapter`: added `torch.nn.Module`, `str`, `None` return type - `transpose`: added `torch.Tensor`, `bool`, and return type - `_is_valid_match`: added missing `bool` return type - `cast_mixed_precision_params`: added `torch.nn.Module`, `torch.dtype`, `None` return type - `match_target_against_key`: added `Optional[re.Match[str]]` return type Built by Rudrendu Paul, developed with Claude Code
|
Thanks for adding these type hints to the two PEFT modules. Inspecting them, they look good to me. One thing I wonder: Did you run any type checker to see if it shows issues with these type hints? I assume not, as I would imagine quite a few PEFT type annotations are not quite right or incomplete, but LMK what you did. |
|
Hi @BenjaminBossan — thanks for the review and the kind words! To answer your question: I did not run a dedicated type checker (mypy/pyright) on these changes at the time of opening. The annotations were derived by reading the function implementations, tracing call sites, and cross-referencing PyTorch/Python type conventions for each function. I'll run pyright on the two changed modules now and reply with the results. If it surfaces any issues with the added annotations I'll address them in a follow-up commit. Happy to do that before you merge — just wanted to be transparent about my process. |
|
@BenjaminBossan — here are the pyright results on the two changed files. Command: merge_utils.py — 1 error:
other.py — 36 errors, breakdown:
Functions I added type hints to (none flagged by pyright):
This confirms your expectation — the codebase already had type-annotation gaps that pyright surfaces, but our new annotations themselves are clean. Happy to address any of the pre-existing issues in a follow-up PR if that would be useful. |
|
@RudrenduPaul thanks for double-checking. One question: Would you be interested in cleaning up some of these type annotation errors? If yes, I'd prefer to have that all in one PR instead of having a trickle of smaller PRs. I wouldn't expect all type errors to be easy to resolve, as this depends on how well typed other packages like torch and transformers are. Moreover, type checkers are also lacking in some regards, though that seems to be improving lately. My expectation is not perfectly annotated code that doesn't throw any typing errors, but at least to fix the most egregious errors. LMK if you're interested. |
|
@BenjaminBossan — absolutely, I'd be happy to clean up more type annotation errors in a single comprehensive PR. That makes sense to keep it consolidated rather than a trickle of smaller PRs. I'll run pyright across the full Happy to include this current PR's changes as part of that broader effort if you'd prefer to close this one and have me resubmit everything together, or I can do the broader cleanup as a follow-up — whatever works best for you. |
This sounds good and the scope should work well too. And yes, let's aim at the most obvious errors but not for 100% correctness, which sometimes requires monstrosities like
I'd say just push it onto this PR. |
|
Thanks @BenjaminBossan — sounds good. I'll keep the scope at |
- loftq_utils.py: initialize quantizer/dequantized_weight/L/R before loop; assert quantizer is not None before use; assert resolved_archive_file and sharded_metadata are not None after get_checkpoint_shard_files - integrations.py: initialize old_register_buffer unconditionally so the finally block is not flagged as possibly-unbound when include_buffers is truthy Built by Rudrendu Paul, developed with Claude Code
|
Pushed the pyright cleanup across
Remaining errors in these files are complex third-party attribute issues ( Let me know if you'd like me to cover additional files in |
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for extending the type annotation. I found a couple of these changes not to be really helpful, which I marked. Please take a look.
| logging.info( | ||
| f"Weight: ({out_feature}, {in_feature}) | Rank: {reduced_rank} | Num Iter: {num_iter} | Num Bits: {num_bits}" | ||
| ) | ||
| quantizer: Optional[NFQuantizer] = None |
There was a problem hiding this comment.
Not a fan of this change, quantizer cannot be None. Let's remove this completely, this part of the code is deprecated anyway.
| dequantized_weight: torch.Tensor = weight | ||
| L: torch.Tensor = weight | ||
| R: torch.Tensor = weight |
There was a problem hiding this comment.
Let's remove these changes too. This is the type of change that I feel is only for satisfying the type checker without any real value.
| ).to(compute_device) | ||
| dequantized_weight = bnb.functional.dequantize_4bit(qweight.data, qweight.quant_state) | ||
| elif num_bits == 4: | ||
| assert quantizer is not None |
| assert resolved_archive_file is not None | ||
| assert sharded_metadata is not None |
There was a problem hiding this comment.
Again, let's remove this, it's unnecessary except to make the type checker happy.
Remove all type-checker-only additions that BenjaminBossan requested to revert: the Optional[NFQuantizer] initialization, pre-loop dequantized_weight/ L/R initializations, assert quantizer is not None, assert resolved_archive_file / sharded_metadata are not None, and the duplicate old_register_buffer assignment inside if include_buffers. Built by Rudrendu Paul, developed with Claude Code
|
Hi @BenjaminBossan — pushed a commit addressing all five of your inline change requests:
The PR is now back to containing only the type-hint additions you approved in the first review, without the assert/initialization noise. Ready for re-review whenever you have a moment. |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
BenjaminBossan
left a comment
There was a problem hiding this comment.
Happy with the changes, thanks for updating.
Continues the pooled type-hint effort in this PR (precedent: huggingface#3144): - gather_params_ctx: Union[nn.Parameter, Iterable[nn.Parameter]] param, Iterator[None] return - dequantize_bnb_weight: Optional[Any] state param, torch.Tensor return - get_layer_device_map / map_cache_to_layer_device_map: Any model param (avoids new pyright errors from nn.Module.__getattr__ ambiguity on dynamically-set hf_device_map/config attributes), transformers.Cache for cache param - init_empty_weights, _init_on_device, _skip_init_on_device: Iterator[None] return - skip_init_on_device: Callable param and return type No functional changes.
What does this PR do?
Adds missing type annotations to utility functions in
src/peft/utils/merge_utils.pyandsrc/peft/utils/other.py, then extends the scope to a pyright cleanup of the fullsrc/peft/utils/directory (at @BenjaminBossan's request in the review thread).The following functions were missing type hints:
merge_utils.pyreshape_weight_task_tensors: addedtorch.Tensorparameter and return typesother.py_get_input_embeddings_name: addedtorch.nn.Module,Optional[str]param/return types_get_submodules: added full param/return types including tuple_get_submodules_with_grandparent: added full param/return types_freeze_adapter: addedtorch.nn.Module,strparam types andNonereturn typetranspose: addedtorch.Tensor,boolparam types_is_valid_match: addedboolreturn typecast_mixed_precision_params: addedtorch.nn.Module,torch.dtypeparam typesmatch_target_against_key: addedOptional[re.Match[str]]return typePyright cleanup across
src/peft/utils/addressed obviouspossibly-unbounderrors as agreed with @BenjaminBossan (skipping complex third-party attribute errors that would require monstrosities likeCallable[Sequence[tuple[str] | None], ...]).Duplicate check
No overlapping open PRs at time of submission.
gh pr list --repo huggingface/peft --state open --search "type hints"returned no conflicting PRs.Tests
Type-hint only change — no behavioral modification. Test commands:
npx pyright src/peft/utils/merge_utils.py src/peft/utils/other.py— 37 errors in total, all in pre-existing code or import resolution (torch/accelerate not installed in bare env); none in the annotated functions (reported to @BenjaminBossan in comments)Before submitting
AI assistance disclosure
This PR was developed with the assistance of Claude Code (AI). All changes have been read, understood, and verified by the human contributor (Rudrendu Paul). The type annotations were derived by reading the function implementations and cross-referencing PyTorch type conventions, as discussed with @BenjaminBossan in the review thread.