From 7091a0016858a6bc794f23a7a352607ad2045cce Mon Sep 17 00:00:00 2001 From: suluyan Date: Mon, 20 Jul 2026 15:45:28 +0800 Subject: [PATCH] fix: resolve cross-repo auto_map when cache paths contain -- modelscope_hub 0.1.x layout embeds -- in local dirs; rejoining that path into class_reference broke transformers' split("--"). Pass the local snapshot as pretrained_model_name_or_path with a bare class name instead, and stop mutating the args tuple in place. Co-authored-by: Cursor --- modelscope/utils/hf_util/patcher.py | 30 +++++++-- tests/utils/test_hf_util.py | 99 +++++++++++++++++++++++++---- 2 files changed, 111 insertions(+), 18 deletions(-) diff --git a/modelscope/utils/hf_util/patcher.py b/modelscope/utils/hf_util/patcher.py index ee26d3d5..2404518c 100644 --- a/modelscope/utils/hf_util/patcher.py +++ b/modelscope/utils/hf_util/patcher.py @@ -161,16 +161,29 @@ def _get_class_from_dynamic_module(class_reference, *args, **kwargs): When a config's ``auto_map`` references another repo, transformers calls ``get_class_from_dynamic_module`` to fetch it. This wrapper ensures that fetch goes through ModelScope instead of HuggingFace. + + Cross-repo ``auto_map`` entries use ``repo_id--module.Class``. After + ``snapshot_download``, the local cache path may itself contain ``--`` + (modelscope_hub 0.1.x layout: ``models/{owner}--{name}/snapshots/...``). + Re-joining that path with ``--`` would make transformers' + ``class_reference.split("--")`` raise ``ValueError``. Instead, pass the + local directory as ``pretrained_model_name_or_path`` and the bare + ``module.Class`` as ``class_reference`` so transformers takes the + ``os.path.isdir`` branch. """ from transformers.dynamic_module_utils import origin_get_class_from_dynamic_module - if 'pretrained_model_name_or_path' in inspect.signature( - origin_get_class_from_dynamic_module).parameters: + has_pretrained_arg = ( + 'pretrained_model_name_or_path' + in inspect.signature(origin_get_class_from_dynamic_module).parameters) + # ``args`` is a tuple; never mutate it in place. + if has_pretrained_arg and args: pretrained_model_name_or_path = args[0] if not os.path.exists(pretrained_model_name_or_path): from modelscope import snapshot_download - args[0] = snapshot_download(pretrained_model_name_or_path) + args = (snapshot_download(pretrained_model_name_or_path), ) + args[1:] if '--' in class_reference: - repo_id, class_reference = class_reference.split('--') + # Only the first ``--`` is the auto_map delimiter (repo vs module). + repo_id, class_reference = class_reference.split('--', 1) if not os.path.exists(repo_id): download_kwargs = {} extra_allow_file_pattern = _decide_allow_file_pattern( @@ -182,7 +195,14 @@ def _get_class_from_dynamic_module(class_reference, *args, **kwargs): download_kwargs['ignore_file_pattern'] = ignore_file_pattern from modelscope import snapshot_download repo_id = snapshot_download(repo_id, **download_kwargs) - class_reference = repo_id + '--' + class_reference + if has_pretrained_arg: + # Local path + bare class name; do not rejoin with ``--``. + args = (repo_id, ) + args[1:] + else: + # Legacy transformers without pretrained_model_name_or_path. + # Unsafe if repo_id (local cache) contains '--'; modern + # transformers always take the branch above. + class_reference = repo_id + '--' + class_reference return origin_get_class_from_dynamic_module(class_reference, *args, **kwargs) diff --git a/tests/utils/test_hf_util.py b/tests/utils/test_hf_util.py index 1dd52b39..80f875e1 100644 --- a/tests/utils/test_hf_util.py +++ b/tests/utils/test_hf_util.py @@ -249,19 +249,92 @@ class HFUtilTest(unittest.TestCase): f'Expected no weight files in {model_dir}, but found: ' f"{[f for f in files if f.endswith('.safetensors') or f.endswith('.bin')]}" ) - cache_dir = os.path.dirname(model_dir) - cache_dir = os.path.dirname(cache_dir) - model_dir_2 = os.path.join(cache_dir, 'nomic-ai', 'nomic-bert-2048') - if os.path.exists(model_dir_2): - files = os.listdir(model_dir_2) - has_weight_files = any( - f.endswith('.safetensors') or f.endswith('.bin') - for f in files) - self.assertFalse( - has_weight_files, - f'Expected no weight files in {model_dir}, but found: ' - f"{[f for f in files if f.endswith('.safetensors') or f.endswith('.bin')]}" - ) + # modelscope_hub 0.1.x layout uses models/{owner}--{name}/...; + # older layout used {owner}/{name}/. Accept either. + cache_root = model_dir + for _ in range(4): + parent = os.path.dirname(cache_root) + if parent == cache_root: + break + cache_root = parent + candidates = [ + os.path.join(cache_root, 'nomic-ai', 'nomic-bert-2048'), + os.path.join(cache_root, 'models', 'nomic-ai--nomic-bert-2048'), + ] + for model_dir_2 in candidates: + if not os.path.exists(model_dir_2): + continue + # Walk into snapshots/{rev} if present. + check_dirs = [model_dir_2] + snapshots = os.path.join(model_dir_2, 'snapshots') + if os.path.isdir(snapshots): + check_dirs.extend( + os.path.join(snapshots, d) + for d in os.listdir(snapshots) + if os.path.isdir(os.path.join(snapshots, d))) + for check_dir in check_dirs: + files = os.listdir(check_dir) + has_weight_files = any( + f.endswith('.safetensors') or f.endswith('.bin') + for f in files) + self.assertFalse( + has_weight_files, + f'Expected no weight files in {check_dir}, but found: ' + f"{[f for f in files if f.endswith('.safetensors') or f.endswith('.bin')]}" + ) + + def test_dynamic_module_double_dash_cache_path(self): + """Cross-repo auto_map must survive cache paths that contain '--'. + + modelscope_hub 0.1.x stores repos under ``models/{owner}--{name}/``. + Rejoining that path into ``class_reference`` with ``--`` makes + transformers' ``split("--")`` raise ValueError. + """ + from unittest import mock + + from modelscope.utils.hf_util.patcher import \ + _get_class_from_dynamic_module + + tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmp, ignore_errors=True) + local_path = os.path.join(tmp, 'models', 'nomic-ai--nomic-bert-2048', + 'snapshots', 'rev') + os.makedirs(local_path) + pretrained = os.path.join(tmp, 'models', + 'nomic-ai--nomic-embed-text-v1.5', + 'snapshots', 'rev') + os.makedirs(pretrained) + + captured = {} + + def fake_origin(class_reference, + pretrained_model_name_or_path, + *args, + **kwargs): + # Signature must match transformers so has_pretrained_arg is True. + captured['class_reference'] = class_reference + captured['pretrained'] = pretrained_model_name_or_path + return type('DummyConfig', (), {}) + + class_ref = ( + 'nomic-ai/nomic-bert-2048--' + 'configuration_hf_nomic_bert.NomicBertConfig') + + # create=True: do not permanently leave origin_* on the module + # (would break test_import_not_pollute_dynamic_module). + with mock.patch( + 'transformers.dynamic_module_utils.origin_get_class_from_dynamic_module', + new=fake_origin, + create=True): + with mock.patch( + 'modelscope.snapshot_download', return_value=local_path): + _get_class_from_dynamic_module(class_ref, pretrained) + + # Must pass bare module.Class (no '--') so transformers does not split. + self.assertEqual(captured['class_reference'], + 'configuration_hf_nomic_bert.NomicBertConfig') + # Local cache path (which contains '--') is pretrained_model_name_or_path. + self.assertEqual(captured['pretrained'], local_path) def test_import_not_pollute_dynamic_module(self): """Importing from modelscope must not globally patch