From 9ab427b29a52f6b3682a36dd3fd44c29734b4d84 Mon Sep 17 00:00:00 2001 From: "Xingjun.Wang" Date: Tue, 21 Jul 2026 17:57:49 +0800 Subject: [PATCH] [Fix] Fix progress_callbacks for snapshot_download func (#1760) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(msdatasets): make HfFileSystem monkey-patch idempotent for repeated streaming loads When streaming=True the patches on HfFileSystem._open and HfFileSystem.__init__ are intentionally kept alive after load_dataset_with_ctx exits. A second call to load_dataset_with_ctx then snaps the already-patched wrappers as 'originals', causing _hf_fs_init_with_cookie / _hf_fs_open to call themselves recursively until RecursionError: maximum recursion depth exceeded. Fix: detect whether HfFileSystem is already patched before overwriting _hf_fs_{open,init}_original, and skip re-applying the patch when it is already in place. The finally-block restores only patches that were applied in the current invocation, leaving pre-existing patches intact. Adds: test_hf_filesystem_patch_idempotent_for_repeated_streaming_loads * fix(msdatasets): restore patches when streaming load fails; hermetic tests Addresses both Gemini Code Assist review comments on PR #1754: 1. Bug fix – patch leak on failed streaming loads: Replace guard with flag. The flag is set True only after load_dataset() succeeds with streaming=True. Any exception in load_dataset (network, auth, invalid dataset, etc.) leaves _streaming_dataset_returned=False, so the finally block always restores all monkey-patches, preventing permanent global-state corruption. 2. Test improvement – hermetic test environment: Add _reset_hf_filesystem_patch() helper that strips pre-existing patches before each unit test, ensuring tests are independent of execution order. Add test_hf_filesystem_patch_restored_when_streaming_load_fails to cover the bug scenario introduced in fix 1. * update requirements for hub * feat(hub): restore progress_callbacks on snapshot_download shim (issue #1757) The progress_callbacks parameter was dropped from snapshot_download after v1.38, breaking legacy code and GUI progress reporting. Re-expose it on modelscope.hub.snapshot_download (positioned before token per the v1.34 signature) and forward it to modelscope_hub.compat. Requires modelscope-hub >= 0.1.8. Add network-free forwarding tests that patch the compat delegate so the real shim executes. * fix lint * feat(hub): add legacy-cache capability guard to snapshot_download shim Warn once (thread-safe) when the installed modelscope-hub lacks pre-1.38 legacy cache auto-detection (DownloadManager._find_legacy_repo_dir, added in modelscope-hub>=0.1.7), so programmatic snapshot_download/dataset_snapshot_download callers are not silently downloading into the new layout while an old cache exists. Capability is probed, not reimplemented (kept single-sourced in modelscope-hub). Adds network-free tests for present/absent capability and fire-once behavior. --- modelscope/hub/snapshot_download.py | 49 ++++++++++++++++++++++++- requirements/hub.txt | 2 +- tests/hub/test_download_callback.py | 32 +++++++++++++++++ tests/hub/test_legacy_cache_guard.py | 54 ++++++++++++++++++++++++++++ 4 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 tests/hub/test_legacy_cache_guard.py diff --git a/modelscope/hub/snapshot_download.py b/modelscope/hub/snapshot_download.py index 1f500ecb..32404ece 100644 --- a/modelscope/hub/snapshot_download.py +++ b/modelscope/hub/snapshot_download.py @@ -4,16 +4,57 @@ Delegates to ``modelscope_hub.compat`` while keeping ``revision``, ``cache_dir`` and friends accessible as positional arguments for backward compatibility. """ from __future__ import annotations +import threading from pathlib import Path -from typing import Dict, List, Optional, Union +from typing import TYPE_CHECKING, Dict, List, Optional, Type, Union from modelscope_hub.compat.snapshot_download import \ dataset_snapshot_download as _compat_dataset_snapshot_download from modelscope_hub.compat.snapshot_download import \ snapshot_download as _compat_snapshot_download +from modelscope.utils.logger import get_logger + +if TYPE_CHECKING: + from .callback import ProgressCallback + +logger = get_logger() + __all__ = ['snapshot_download', 'dataset_snapshot_download'] +# Capability probe: pre-1.38 cache auto-detection lives in modelscope-hub +# (DownloadManager._find_legacy_repo_dir, added in modelscope-hub>=0.1.7). +# Warn once if the installed hub predates it, so an existing legacy cache is +# not silently ignored and re-downloaded into the new layout. +_legacy_cache_capability: Optional[bool] = None +_legacy_cache_lock = threading.Lock() + + +def _warn_if_legacy_cache_detection_unavailable() -> None: + """Warn once when the installed modelscope-hub cannot auto-detect a + pre-1.38 (legacy) cache layout, so downloads don't silently skip an + existing local cache and re-fetch into the new layout. + """ + global _legacy_cache_capability + if _legacy_cache_capability is not None: + return + with _legacy_cache_lock: + if _legacy_cache_capability is not None: + return + try: + from modelscope_hub._download import DownloadManager + _legacy_cache_capability = hasattr(DownloadManager, + '_find_legacy_repo_dir') + except Exception: + _legacy_cache_capability = False + if not _legacy_cache_capability: + logger.warning( + 'The installed modelscope-hub lacks legacy cache ' + 'auto-detection (added in modelscope-hub>=0.1.7). An existing ' + 'pre-1.38 cache will not be reused; files will be downloaded ' + 'into the new cache layout. Upgrade with: ' + "pip install -U 'modelscope-hub>=0.1.8'.") + def snapshot_download( model_id: Optional[str] = None, @@ -30,6 +71,7 @@ def snapshot_download( max_workers: Optional[int] = None, repo_id: Optional[str] = None, repo_type: Optional[str] = None, + progress_callbacks: Optional[List[Type[ProgressCallback]]] = None, token: Optional[str] = None, endpoint: Optional[str] = None, ) -> str: @@ -37,7 +79,10 @@ def snapshot_download( Preserves the legacy positional-argument signature for backward compatibility while delegating to ``modelscope_hub.compat``. + ``progress_callbacks`` is a list of :class:`ProgressCallback` subclasses + (not instances), each instantiated per file to report download progress. """ + _warn_if_legacy_cache_detection_unavailable() return _compat_snapshot_download( model_id=model_id, revision=revision, @@ -56,6 +101,7 @@ def snapshot_download( local_files_only=bool(local_files_only) if local_files_only is not None else False, user_agent=user_agent, + progress_callbacks=progress_callbacks, ) @@ -75,6 +121,7 @@ def dataset_snapshot_download( endpoint: Optional[str] = None, ) -> str: """Download a dataset repo snapshot (legacy positional-arg signature).""" + _warn_if_legacy_cache_detection_unavailable() effective_id = dataset_id or repo_id return _compat_dataset_snapshot_download( dataset_id=effective_id, diff --git a/requirements/hub.txt b/requirements/hub.txt index 4b52c05d..0f9ac799 100644 --- a/requirements/hub.txt +++ b/requirements/hub.txt @@ -1,5 +1,5 @@ filelock -modelscope-hub>=0.0.7 +modelscope-hub>=0.1.8 packaging requests>=2.25 setuptools diff --git a/tests/hub/test_download_callback.py b/tests/hub/test_download_callback.py index 598c2ac4..9144a1e2 100644 --- a/tests/hub/test_download_callback.py +++ b/tests/hub/test_download_callback.py @@ -1,5 +1,6 @@ import tempfile import unittest +from unittest import mock from tqdm import tqdm @@ -48,5 +49,36 @@ class ProgressCallbackTest(unittest.TestCase): print(f'model_dir: {model_dir}') +class SnapshotDownloadForwardTest(unittest.TestCase): + """Network-free tests: the shim forwards progress_callbacks to compat.""" + + # ``modelscope.hub.snapshot_download`` the attribute is shadowed by the + # re-exported function, so patch via the fully qualified module string. + _COMPAT_TARGET = \ + 'modelscope.hub.snapshot_download._compat_snapshot_download' + + def test_progress_callbacks_forwarded_to_compat(self): + from modelscope.hub.snapshot_download import snapshot_download + + with mock.patch( + self._COMPAT_TARGET, return_value='/tmp/snapshot') as m: + result = snapshot_download( + 'owner/repo', progress_callbacks=[NewProgressCallback]) + + self.assertEqual(result, '/tmp/snapshot') + _, kwargs = m.call_args + self.assertEqual(kwargs['progress_callbacks'], [NewProgressCallback]) + + def test_progress_callbacks_default_none(self): + from modelscope.hub.snapshot_download import snapshot_download + + with mock.patch( + self._COMPAT_TARGET, return_value='/tmp/snapshot') as m: + snapshot_download('owner/repo') + + _, kwargs = m.call_args + self.assertIsNone(kwargs['progress_callbacks']) + + if __name__ == '__main__': unittest.main() diff --git a/tests/hub/test_legacy_cache_guard.py b/tests/hub/test_legacy_cache_guard.py new file mode 100644 index 00000000..b1cdf66b --- /dev/null +++ b/tests/hub/test_legacy_cache_guard.py @@ -0,0 +1,54 @@ +import importlib +import sys +import unittest +from unittest import mock + + +class LegacyCacheGuardTest(unittest.TestCase): + """Tests for the modelscope-hub capability guard in the shim. + + Network-free: only probes whether the loaded modelscope-hub exposes the + legacy-cache auto-detection capability and warns once when it does not. + """ + + def setUp(self): + # NOTE: ``modelscope.hub.snapshot_download`` the *attribute* is shadowed + # by the re-exported function in ``modelscope.hub.__init__``; use + # importlib to obtain the actual submodule object. + sd = importlib.import_module('modelscope.hub.snapshot_download') + self.sd = sd + # Reset the fire-once probe cache before each test. + sd._legacy_cache_capability = None + + def tearDown(self): + self.sd._legacy_cache_capability = None + + def test_capability_present_no_warning(self): + sd = self.sd + # The real modelscope-hub DownloadManager has _find_legacy_repo_dir. + with mock.patch.object(sd.logger, 'warning') as warn: + sd._warn_if_legacy_cache_detection_unavailable() + self.assertTrue(sd._legacy_cache_capability) + warn.assert_not_called() + + def test_capability_absent_warns_once(self): + sd = self.sd + + class _OldDownloadManager: # lacks _find_legacy_repo_dir + pass + + fake_module = mock.MagicMock() + fake_module.DownloadManager = _OldDownloadManager + + with mock.patch.dict(sys.modules, + {'modelscope_hub._download': fake_module}): + with mock.patch.object(sd.logger, 'warning') as warn: + sd._warn_if_legacy_cache_detection_unavailable() + sd._warn_if_legacy_cache_detection_unavailable() + + self.assertFalse(sd._legacy_cache_capability) + self.assertEqual(warn.call_count, 1) # fire-once + + +if __name__ == '__main__': + unittest.main()