[Fix] Fix progress_callbacks for snapshot_download func (#1760)

* 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

@@ -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,

requirements/hub.txt

@@ -1,5 +1,5 @@
 filelock
-modelscope-hub>=0.0.7
+modelscope-hub>=0.1.8
 packaging
 requests>=2.25
 setuptools

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()

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()