diff --git a/docker/build_image.py b/docker/build_image.py index 8fab7f96..513a34b6 100644 --- a/docker/build_image.py +++ b/docker/build_image.py @@ -1,5 +1,4 @@ import argparse -import json import os import platform import re @@ -10,6 +9,8 @@ from copy import copy from datetime import datetime from typing import Any, List, Optional +import json + docker_registry = os.environ['DOCKER_REGISTRY'] assert docker_registry, 'You must pass a valid DOCKER_REGISTRY' timestamp = datetime.now() @@ -507,16 +508,15 @@ class AmdImageBuilder(Builder): @classmethod def _fetch_rocm_tags(cls, page_size: int = 100) -> List[dict]: tags: List[dict] = [] - url = ( - f'https://hub.docker.com/v2/repositories/{VLLM_ROCM_REPO}/tags' - f'?page_size={page_size}&ordering=-last_updated') + url = (f'https://hub.docker.com/v2/repositories/{VLLM_ROCM_REPO}/tags' + f'?page_size={page_size}&ordering=-last_updated') while url: req = urllib.request.Request( url, headers={'User-Agent': 'modelscope-docker-builder'}) try: with urllib.request.urlopen(req, timeout=60) as resp: payload = json.load(resp) - except urllib.error.URLError as exc: + except (urllib.error.URLError, json.JSONDecodeError) as exc: raise RuntimeError( f'Failed to query Docker Hub tags for {VLLM_ROCM_REPO}: ' f'{exc}') from exc @@ -569,8 +569,8 @@ class AmdImageBuilder(Builder): def init_args(self, args: Any) -> Any: # Auto-discover from Docker Hub unless an explicit override is given. override = getattr(args, 'base_image_tag', None) - if override and str(override).strip() and str(override).strip().lower( - ) not in {'auto', 'latest'}: + if override and str(override).strip() and str( + override).strip().lower() not in {'auto', 'latest'}: args.base_image_tag = str(override).strip() if not self._is_specific_release_tag(args.base_image_tag): raise ValueError( @@ -630,9 +630,9 @@ class AmdImageBuilder(Builder): ' if os.environ.get(k): info[k.lower()]=os.environ[k]\n' 'print(json.dumps(info))\n') for py in ('python3', 'python'): - result = cls._run_capture( - 'docker', 'run', '--rm', '--network', 'none', '--entrypoint', - py, base_image, '-c', script) + result = cls._run_capture('docker', 'run', '--rm', '--network', + 'none', '--entrypoint', py, base_image, + '-c', script) if result.returncode == 0 and result.stdout.strip(): try: return json.loads(result.stdout.strip().splitlines()[-1]) @@ -714,9 +714,11 @@ class AmdImageBuilder(Builder): versions = { 'rocm': cls._normalize_version(rocm) if rocm else None, - 'python': cls._normalize_version(python_ver) if python_ver else None, + 'python': + cls._normalize_version(python_ver) if python_ver else None, 'torch': cls._normalize_version(torch_ver) if torch_ver else None, - 'ubuntu': cls._normalize_version(ubuntu_ver) if ubuntu_ver else None, + 'ubuntu': + cls._normalize_version(ubuntu_ver) if ubuntu_ver else None, } print('Probed AMD base image versions:') for key, value in versions.items(): @@ -744,9 +746,8 @@ class AmdImageBuilder(Builder): raise RuntimeError( 'AMD image tag requires probed rocm/python/torch versions. ' f'Got rocm={rocm}, python={py_tag}, torch={torch}') - return ( - f'{docker_registry}:ubuntu{ubuntu}-rocm{rocm}-{py_tag}-' - f'torch{torch}-{self.args.modelscope_version}-test') + return (f'{docker_registry}:ubuntu{ubuntu}-rocm{rocm}-{py_tag}-' + f'torch{torch}-{self.args.modelscope_version}-test') def _log_base_image_info(self) -> int: base_image = self.args.base_image @@ -767,8 +768,8 @@ class AmdImageBuilder(Builder): print(f'AMD base image inspect warning: {result.stderr.strip()}') versions = self.probe_base_image_versions(base_image) - if not versions.get('rocm') or not versions.get('python') or not versions.get( - 'torch'): + if not versions.get('rocm') or not versions.get( + 'python') or not versions.get('torch'): print('ERROR: failed to probe rocm/python/torch from base image') return 1 self.args.amd_rocm_version = versions['rocm'] @@ -779,9 +780,7 @@ class AmdImageBuilder(Builder): self.args.amd_ubuntu_version = versions['ubuntu'] else: self.args.amd_ubuntu_version = self.args.ubuntu_version - print( - f'AMD output image tag will be: {self.image()}' - ) + print(f'AMD output image tag will be: {self.image()}') print('=' * 60) return 0 @@ -801,10 +800,9 @@ class AmdImageBuilder(Builder): rocm = self.args.amd_rocm_version py_tag = self.args.amd_python_tag torch = self.args.amd_torch_version - image_tag2 = ( - f'{docker_registry}:ubuntu{ubuntu}-rocm{rocm}-{py_tag}-' - f'torch{torch}-{self.args.modelscope_version}-' - f'{formatted_time}-test') + image_tag2 = (f'{docker_registry}:ubuntu{ubuntu}-rocm{rocm}-{py_tag}-' + f'torch{torch}-{self.args.modelscope_version}-' + f'{formatted_time}-test') ret = self.run_cmd('docker', 'tag', image_name, image_tag2) if ret != 0: return ret diff --git a/modelscope/utils/automodel_utils.py b/modelscope/utils/automodel_utils.py index 73175dc6..1c46b296 100644 --- a/modelscope/utils/automodel_utils.py +++ b/modelscope/utils/automodel_utils.py @@ -147,12 +147,13 @@ def check_model_from_owner_group(model_dir: str, if group in owner_group: return True # Also check cache path pattern: {cache_root}/{owner}--{model_name}/snapshots/{revision} - # Require exactly "{owner}--{name}" format (2 segments split by --) - # to prevent spoofing via accounts like "iic--hacked" which would - # produce paths like "iic--hacked--evil" and bypass the check. + # Require exactly "{owner}--{name}" with both segments non-empty + # to prevent spoofing via accounts like "iic--hacked" (paths like + # "iic--hacked--evil") or empty names like "iic--". grandparent = os.path.basename(os.path.dirname(parent_dir)) if '--' in grandparent: parts = grandparent.split('--') - if len(parts) == 2 and parts[0] in owner_group: + # Both owner and name must be non-empty; reject "iic--" / "--name". + if len(parts) == 2 and all(parts) and parts[0] in owner_group: return True return False diff --git a/modelscope/utils/hf_util/patcher.py b/modelscope/utils/hf_util/patcher.py index ee26d3d5..790eb02c 100644 --- a/modelscope/utils/hf_util/patcher.py +++ b/modelscope/utils/hf_util/patcher.py @@ -161,16 +161,43 @@ 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: - 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) + has_pretrained_arg = ( + 'pretrained_model_name_or_path' + in inspect.signature(origin_get_class_from_dynamic_module).parameters) + # Resolve pretrained_model_name_or_path from kwargs or positional args. + # ``args`` is a tuple; never mutate it in place. + pretrained_in_kwargs = False + pretrained_model_name_or_path = None + if has_pretrained_arg: + if 'pretrained_model_name_or_path' in kwargs: + pretrained_model_name_or_path = kwargs[ + 'pretrained_model_name_or_path'] + pretrained_in_kwargs = True + elif args: + pretrained_model_name_or_path = args[0] + if (pretrained_model_name_or_path is not None + and not os.path.exists(pretrained_model_name_or_path)): + from modelscope import snapshot_download + downloaded_path = snapshot_download(pretrained_model_name_or_path) + if pretrained_in_kwargs: + kwargs['pretrained_model_name_or_path'] = downloaded_path + else: + args = (downloaded_path, ) + args[1:] + pretrained_model_name_or_path = downloaded_path 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 +209,18 @@ 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 ``--``. + # Keep kwargs/positional form consistent with the original call. + if pretrained_in_kwargs: + kwargs['pretrained_model_name_or_path'] = repo_id + else: + 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/modelscope/utils/plugins.py b/modelscope/utils/plugins.py index fa4cb877..cd8a2135 100644 --- a/modelscope/utils/plugins.py +++ b/modelscope/utils/plugins.py @@ -374,6 +374,30 @@ def create_module_from_files(file_list, file_prefix, module_name): importlib.invalidate_caches() +def _module_name_from_model_dir(model_dir: str) -> str: + """Derive a valid Python package name from a local model directory. + + modelscope_hub 0.1.x returns paths like + ``{cache}/models/{owner}--{name}/snapshots/{revision}``. Using + ``Path(model_dir).stem`` on a revision such as ``v1.0.4`` yields + ``v1.0``, and ``importlib.import_module('v1.0.xxx')`` fails with + ``ModuleNotFoundError: No module named 'v1'`` because dots are package + separators. Prefer the ``{owner}--{name}`` segment (plus revision for + isolation) and sanitize to a valid identifier. + """ + path = Path(model_dir).resolve() + if path.parent.name == 'snapshots': + base = f'{path.parent.parent.name}__{path.name}' + else: + # Use the full directory name, not .stem, so dotted names are kept + # intact before sanitization (stem would turn ``v1.0.4`` into ``v1.0``). + base = path.name + module_name = re.sub(r'[^0-9A-Za-z_]', '_', base) + if not module_name or module_name[0].isdigit(): + module_name = f'm_{module_name}' + return module_name + + def import_module_from_model_dir(model_dir): """ import all the necessary module from a model dir @@ -383,7 +407,6 @@ def import_module_from_model_dir(model_dir): No returns, raise error if failed """ - from pathlib import Path file_scanner = FilesAstScanning() file_scanner.traversal_files(model_dir, include_init=True) file_dirs = file_scanner.file_dirs @@ -395,7 +418,7 @@ def import_module_from_model_dir(model_dir): if BASE_MODULE_DIR not in sys.path: sys.path.append(BASE_MODULE_DIR) - module_name = Path(model_dir).stem + module_name = _module_name_from_model_dir(model_dir) # in order to keep forward compatibility, we add module path to # sys.path so that submodule can be imported directly as before diff --git a/tests/msdatasets/test_stream_load.py b/tests/msdatasets/test_stream_load.py index 87bbf096..4ed0c3f3 100644 --- a/tests/msdatasets/test_stream_load.py +++ b/tests/msdatasets/test_stream_load.py @@ -44,15 +44,12 @@ class TestStreamLoad(unittest.TestCase): pass self.assertIs(HfFileSystem._open, hf_datasets_util._hf_fs_open) - self.assertIsNot( - hf_datasets_util._hf_fs_open_original, - hf_datasets_util._hf_fs_open) - self.assertIs( - HfFileSystem.__init__, - hf_datasets_util._hf_fs_init_with_cookie) - self.assertIsNot( - hf_datasets_util._hf_fs_init_original, - hf_datasets_util._hf_fs_init_with_cookie) + self.assertIsNot(hf_datasets_util._hf_fs_open_original, + hf_datasets_util._hf_fs_open) + self.assertIs(HfFileSystem.__init__, + hf_datasets_util._hf_fs_init_with_cookie) + self.assertIsNot(hf_datasets_util._hf_fs_init_original, + hf_datasets_util._hf_fs_init_with_cookie) finally: HfFileSystem._open = hf_fs_open_before HfFileSystem.__init__ = hf_fs_init_before @@ -75,7 +72,8 @@ class TestStreamLoad(unittest.TestCase): 'load_dataset', side_effect=RuntimeError('load failed')): with self.assertRaises(RuntimeError): - with hf_datasets_util.load_dataset_with_ctx(streaming=True): + with hf_datasets_util.load_dataset_with_ctx( + streaming=True): pass self.assertIs(HfFileSystem._open, hf_fs_open_clean) diff --git a/tests/utils/test_hf_util.py b/tests/utils/test_hf_util.py index 1dd52b39..4e98d57f 100644 --- a/tests/utils/test_hf_util.py +++ b/tests/utils/test_hf_util.py @@ -249,19 +249,182 @@ 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_dynamic_module_remote_pretrained_tuple_args(self): + """Remote pretrained_model_name_or_path must not mutate args in place. + + ``*args`` is a tuple; ``args[0] = snapshot_download(...)`` raises + TypeError. Rebuild the tuple instead (regression from 9379504f). + """ + 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) + downloaded = os.path.join(tmp, 'models', 'org--model', 'snapshots', + 'rev') + os.makedirs(downloaded) + + captured = {} + + def fake_origin(class_reference, pretrained_model_name_or_path, *args, + **kwargs): + captured['class_reference'] = class_reference + captured['pretrained'] = pretrained_model_name_or_path + return type('DummyConfig', (), {}) + + # No '--' in class_reference: only the pretrained download branch runs. + remote_id = 'org/model-not-on-disk' + 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=downloaded) as sd: + # Must not raise TypeError: 'tuple' object does not support + # item assignment. + _get_class_from_dynamic_module('modeling.Foo', remote_id) + + sd.assert_called_once_with(remote_id) + self.assertEqual(captured['class_reference'], 'modeling.Foo') + self.assertEqual(captured['pretrained'], downloaded) + + def test_dynamic_module_pretrained_via_kwargs(self): + """pretrained_model_name_or_path may be passed as a keyword argument.""" + 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) + downloaded = os.path.join(tmp, 'models', 'org--model', 'snapshots', + 'rev') + cross_repo = os.path.join(tmp, 'models', 'org--other', 'snapshots', + 'rev') + os.makedirs(downloaded) + os.makedirs(cross_repo) + + captured = {} + + def fake_origin(class_reference, pretrained_model_name_or_path, *args, + **kwargs): + captured['class_reference'] = class_reference + captured['pretrained'] = pretrained_model_name_or_path + captured['kwargs'] = kwargs + return type('DummyConfig', (), {}) + + remote_id = 'org/model-not-on-disk' + class_ref = 'org/other--configuration_foo.FooConfig' + + def fake_download(repo_id, **kwargs): + if repo_id == remote_id: + return downloaded + if repo_id == 'org/other': + return cross_repo + raise AssertionError(f'unexpected download: {repo_id}') + + with mock.patch( + 'transformers.dynamic_module_utils.origin_get_class_from_dynamic_module', + new=fake_origin, + create=True): + with mock.patch( + 'modelscope.snapshot_download', side_effect=fake_download): + # Keyword form: must download and not pass duplicate positional. + _get_class_from_dynamic_module( + class_ref, pretrained_model_name_or_path=remote_id) + + self.assertEqual(captured['class_reference'], + 'configuration_foo.FooConfig') + self.assertEqual(captured['pretrained'], cross_repo) + self.assertNotIn('pretrained_model_name_or_path', captured['kwargs']) def test_import_not_pollute_dynamic_module(self): """Importing from modelscope must not globally patch diff --git a/tests/utils/test_owner_group_path_safety.py b/tests/utils/test_owner_group_path_safety.py new file mode 100644 index 00000000..9a0bb5c6 --- /dev/null +++ b/tests/utils/test_owner_group_path_safety.py @@ -0,0 +1,30 @@ +# Copyright (c) Alibaba, Inc. and its affiliates. +import unittest + +from modelscope.utils.automodel_utils import check_model_from_owner_group + + +class OwnerGroupPathSafetyTest(unittest.TestCase): + """Safety checks for trusted-owner cache path recognition.""" + + def test_empty_name_cache_path_rejected(self): + # modelscope_hub layout: {cache}/{owner}--{name}/snapshots/{rev} + # Empty name ("iic--") must not be treated as a trusted owner path. + self.assertFalse( + check_model_from_owner_group('/cache/iic--/snapshots/v1')) + self.assertFalse( + check_model_from_owner_group('/cache/damo--/snapshots/v1')) + + def test_valid_and_spoof_cache_paths(self): + self.assertTrue( + check_model_from_owner_group('/cache/iic--x/snapshots/v1')) + self.assertFalse( + check_model_from_owner_group('/cache/--iic/snapshots/v1')) + self.assertFalse( + check_model_from_owner_group( + '/cache/iic--hacked--evil/snapshots/v1')) + self.assertTrue(check_model_from_owner_group('/cache/iic/some_model')) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/utils/test_plugin.py b/tests/utils/test_plugin.py index bb01886a..fa0e2872 100644 --- a/tests/utils/test_plugin.py +++ b/tests/utils/test_plugin.py @@ -127,5 +127,40 @@ class PluginTest(unittest.TestCase): self.assertEqual(len(result.items()), len(OFFICIAL_PLUGINS)) +class ModuleNameFromModelDirTest(unittest.TestCase): + """Regression: snapshot revision paths must not become dotted module names.""" + + def test_snapshot_revision_with_dots(self): + from modelscope.utils.plugins import _module_name_from_model_dir + + tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmp, ignore_errors=True) + model_dir = os.path.join(tmp, 'models', + 'iic--cv_unet_skin_retouching_torch', + 'snapshots', 'v1.0.4') + os.makedirs(model_dir) + + name = _module_name_from_model_dir(model_dir) + self.assertNotIn('.', name) + self.assertTrue( + name.startswith('iic__cv_unet_skin_retouching_torch__v1_0_4')) + # Old Path(model_dir).stem produced 'v1.0', which importlib treats as + # package 'v1' → ModuleNotFoundError: No module named 'v1'. + self.assertNotEqual(name, 'v1.0') + self.assertFalse(name.startswith('v1')) + + def test_flat_cache_layout_unchanged(self): + from modelscope.utils.plugins import _module_name_from_model_dir + + tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmp, ignore_errors=True) + model_dir = os.path.join(tmp, 'iic', 'cv_unet_skin_retouching_torch') + os.makedirs(model_dir) + + self.assertEqual( + _module_name_from_model_dir(model_dir), + 'cv_unet_skin_retouching_torch') + + if __name__ == '__main__': unittest.main()