Repository navigation
Conversation
Signed-off-by: Yu Yao <yaoyu.094@gmail.com>
Contributor
Author
|
/ok to test 4da9918 |
Contributor
This branch was successfully deployed
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.
The documented PEFT selectors for layers 0 and 1 also match layers 10–19 because
wildcard_matchtreats a literal path separator (.) as a regex wildcard. This can silently attach adapters to extra layers, or exclude those layers unexpectedly.Escape periods before expanding
*. A regression test exercises the existingModuleMatchertarget, exclusion and canonical-mapping paths over 24 layers. The production change is one line.Evidence: the documented layer selectors and matcher helper. This follows the selector-contract investigation prompted by Automodel #4045; no implementation was copied. Related issue #1738 concerns unmatched targets, while this fixes overmatching. The candidate was carried forward from the weekly landscape report.
Validation on base
2161fe922dc97e503eb1d52d644483066d8e99b0, using the pinned MCore dependency and CPU tensors:uv run python -m pytest tests/unit_tests/peft/test_module_matcher.py::TestModuleMatcher::test_layer_wildcards_preserve_path_separators -q— 3 assertion failures before the production change; 3 passed afterward. The initial target/canonical selection included layers 10–19, and the exclusion path incorrectly omitted them.uv run python -m pytest tests/unit_tests/peft/test_module_matcher.py tests/unit_tests/peft/test_utils.py -k 'TestModuleMatcher or wildcard_match' -q— 37 passed, 119 deselected.uv run pre-commit run --all-filesandgit diff --checkpassed.uv run mypy --strict --no-incremental src/megatron/bridge/peft/utils.py tests/unit_tests/peft/test_module_matcher.pyreports 139 existing diagnostics on both the base and patched trees, with identical diagnostics after accounting for line offsets.Current
mainwas rechecked ataea5c536903456835dabdeee7cb0925ba8208bdc; the intervening PP-local export change does not touch the PEFT source, tests or documentation. No matching issue/PR implementation was found.Non-goals: expanding the matcher dialect, changing adapter/model scope, adding dependencies, modifying CI or public conversion signatures, or validating distributed training. This Draft fixes the documented layer-selection contract only.