Repository navigation
lora: list valid target names in the error and log what was adapted (follow-up to #9) - #10
Open
balajiadarsha wants to merge 3 commits into
Open
balajiadarsha wants to merge 3 commits into
balajiadarsha wants to merge 3 commits into
Conversation
training.lora_target_modules was written to the adapter sidecar but never passed to apply_lora_torchtune, so every nn.Linear except lm_head was adapted regardless of the setting. Setting it via Hydra also crashed save_lora_adapter, because json cannot serialize an OmegaConf ListConfig. apply_lora_torchtune now takes target_modules (leaf names, matched like exclude) and raises on a name that matches no layer, an empty list, or a bare string. ZoneATrainer and ZoneDTrainer pass the config value through. With target_modules unset the result is unchanged (state dict bit-identical on a seeded toy backbone). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GmpnEoNqJpQjR59ie7ZMek
11 of 20 tasks
Review follow-ups on #9. - Move layer selection and the target_modules name checks out of apply_lora_torchtune into _select_lora_targets, which needs no torchtune. The selection and bad-name tests now run in CI (which does not install torchtune); only the LoRALinear and sidecar tests still skip without it. - Convert lora_target_modules from an OmegaConf ListConfig to a plain list in build_training_config, so the config the trainers dump into training_state.json records a list rather than its string form. A bare string passes through unchanged, so apply_lora_torchtune still rejects it. - Update the BioReason demo notebook, which still described lora_target_modules as metadata only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When lora_target_modules names a layer that matches nothing, _select_lora_targets now lists the leaf names that can be targeted in the ValueError, e.g. "Available: ['down_proj', 'gate_proj', 'k_proj', ...]". The names come from the backbone actually being adapted, so the list is right for any architecture, and the check is torchtune-free so its test runs in CI. The startup log line in apply_lora_torchtune now breaks the replaced layers down by leaf name, e.g. "replaced 32 linear layers (q_proj: 16, v_proj: 16)", so every run's log shows which layers got adapters. The notebook's recorded output is updated to the new format. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
balajiadarsha
force-pushed
the
lora-target-diagnostics
branch
from
October 15, 2026 20:33
df208b9 to
59064b6
Compare
11 of 20 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to #9, which makes
training.lora_target_modulestake effect. This branch is built on #9's latest commit (5c9e8c2), so merge #9 first. Until then the diff below also shows #9's changes. The only new commit is the last one (59064b6).#9 checks target names and raises
ValueErrorwhen a name matches nothing, but the error doesn't say which names would work, and the run log doesn't say which layers actually got adapters. This PR adds both:_select_lora_targets, the torchtune-free helper from lora: honor lora_target_modules and fix the sidecar save crash #9). When a target matches no non-excludednn.Linear, the message now lists the leaf names that can be targeted:apply_lora_torchtune). The existing startup line now breaks the replaced layers down by leaf name:Which layers get selected is unchanged: the selection loop in
_select_lora_targetsis split into a list of eligible layers and the filtered list built from it, giving the same result. The notebook's recordedapply_lora_torchtuneoutput (bioreason_demo.ipynb, section 9b) is updated to the new log format.Type of Change
Compatibility
src.*import pathsprismCLI surfaceOnly the text of one error message and one INFO log line changes.
How Has This Been Tested?
Aurora login node, CPU only. Rerun after rebasing onto #9's
5c9e8c2:New tests:
test_unmatched_error_lists_available_names, for a typo and for the excludedlm_head. It calls_select_lora_targets, so it runs in CI.test_log_reports_adapted_names, for the default (all layers) and for a restricted list. It needs torchtune, so CI skips it.End-to-end: #9's
5c9e8c2was run end to end on Aurora (SFT with[q_proj, v_proj], GRPO with and without targets, eval, resume) and passed. This commit only changes an error message and a log line, so it has not had its own training run yet.test_log_reports_adapted_namesneeds torchtune, whichrequirements/ci.txtdoesn't install.New Dependencies, Data, or Assets
None.
Checklist
ruff check, ruff 0.16.9 perrequirements/ci.txt)AI/LLM Assistance
Written with Claude Code (Claude Opus 5.5).
🤖 Generated with Claude Code