[b/307966230] Implement force_download option for model_download over HTTP #44

lucyhe · 2023-12-14T01:31:12Z

Previously, after a model or file was downloaded with model_download, there was no way to programmatically force a new download. This PR enables forced downloads.

More Details

This feature can only be used outside of Kaggle notebooks. Kaggle notebooks rely on a different caching mechanism where force_download is not yet possible
force_download defaults to False
If a file is force downloaded, the other files in the same folder are not force downloaded

New flag usage:

kagglehub.model_download(model_handle, force_download=True)
kagglehub.model_download(model_handle, path=path, force_download=True)

lucyhe · 2023-12-14T01:31:33Z

tests/test_cache.py

@@ -26,6 +41,7 @@

 class TestCache(BaseTestCase):
    def test_load_from_cache_miss(self):
+        # Why is this ModelHandle needed?


Was curious about this! Will remove this comment.

Looks like I left this by mistake. Probably when I moved it over to a "constant" at the top 🤦 . You can remove.

lucyhe · 2023-12-14T01:31:40Z

tests/test_cache.py

            os.makedirs(cache_path)
            Path(os.path.join(cache_path, TEST_FILEPATH)).touch()  # Create file

            self.assertEqual(None, load_from_cache(TEST_MODEL_HANDLE, path=TEST_FILEPATH))

    def test_load_from_cache_with_complete_marker_no_files_miss(self):
        with create_test_cache():
+            # Why is this line needed?


Was curious about this too! Will remove this comment.

You can remove it and same for the line #88 below. Another 🤦. Thanks for cleaning this up :)

lucyhe · 2023-12-14T01:32:16Z

I refactored some tests so that the new force_download tests wouldnt feel too verbose. Please let me know if you disagree with any refactoring, and I'm happy to revert!

rosbo

Good work! Just a few minor suggestions/comments.

rosbo · 2023-12-14T16:30:08Z

tests/test_cache.py

@@ -26,6 +41,7 @@

 class TestCache(BaseTestCase):
    def test_load_from_cache_miss(self):
+        # Why is this ModelHandle needed?


Looks like I left this by mistake. Probably when I moved it over to a "constant" at the top 🤦 . You can remove.

rosbo · 2023-12-14T16:34:29Z

tests/test_cache.py

            os.makedirs(cache_path)
            Path(os.path.join(cache_path, TEST_FILEPATH)).touch()  # Create file

            self.assertEqual(None, load_from_cache(TEST_MODEL_HANDLE, path=TEST_FILEPATH))

    def test_load_from_cache_with_complete_marker_no_files_miss(self):
        with create_test_cache():
+            # Why is this line needed?


You can remove it and same for the line #88 below. Another 🤦. Thanks for cleaning this up :)

rosbo · 2023-12-14T16:38:43Z

tests/test_cache.py

+    def test_delete_from_cache(self):
+        with create_test_cache() as d:
+            cache_path = get_cached_path(TEST_MODEL_HANDLE)
+            os.makedirs(cache_path)


Can you add a few files with Path(..).touch() and also files in subdirectories and then assert below that all the files have indeed been deleted.

rosbo · 2023-12-14T16:39:33Z

tests/test_cache.py

+
+            deleted_path = delete_from_cache(TEST_MODEL_HANDLE, path=TEST_FILEPATH)
+
+            self.assertEqual(


Can you assert that the file has been deleted.

rosbo · 2023-12-14T16:52:40Z

tests/test_kaggle_cache_model_download.py

+    def test_versioned_model_download_force_download_raises(self):
+        with create_test_jwt_http_server(KaggleJwtHandler):
+            with self.assertRaises(ValueError):
+                kagglehub.model_download(VERSIONED_MODEL_HANDLE, force_download="hiksjdhf")


Why using hiksjdhf instead of True?

Maybe add a test case that if force_download=False, it still works.

Oops, "hiksjdhf" was a debugging typo, thanks for catching! Added the test case!

rosbo · 2023-12-14T16:53:23Z

tests/test_http_model_download.py

Thanks for adding exhaustive tests & refactoring the assertions!

rosbo · 2023-12-14T16:56:39Z

And also, looks like shutil.rmtree is failing for some versions of Python based on whether files are there or not. See the tests run from CI.

lucyhe · 2023-12-14T17:04:23Z

And also, looks like shutil.rmtree is failing for some versions of Python based on whether files are there or not. See the tests run from CI.

Thanks for the quick review! Are the CI checks identical except for python version? Want to make sure I've narrowed down what could be happening

rosbo · 2023-12-14T22:28:57Z

tests/test_cache.py

@@ -132,3 +130,85 @@ def test_model_archive_path(self):
                ),
                archive_path,
            )
+
+    def _download_test_model_to_cache(self):
+        cache_path = get_cached_path(TEST_MODEL_HANDLE)


You are missing the with create_test_cache(): block (see the other tests above).

This test util function creates a temporary directory to read/write cache files and set the proper environment variable to ensure the cache uses that temporary directory:

kagglehub/tests/utils.py

Lines 25 to 28 in c1b6e00

def create_test_cache():

with TemporaryDirectory() as d:

with mock.patch.dict(os.environ, {CACHE_FOLDER_ENV_VAR_NAME: d}):

yield d

Thanks for the pointer!! I think I misunderstood along the way. While testing I thought I might not need that because I call this helper from within a with create_tech_cache(): block

My bad. Disregard my comment. I didn't realize this was a helper function and not a test.

If this is always called within the create_test_cache block, then you are good.

rosbo · 2023-12-15T00:41:58Z

src/kagglehub/kaggle_cache_resolver.py

+    def __call__(self, h: ModelHandle, path: Optional[str] = None, *, force_download: Optional[bool] = False) -> str:
+        if force_download:
+            msg = "Invalid input: Cannot force download in a Kaggle notebook"
+            raise ValueError(msg)


Actually, we should simply log a warning here.

The reason is that we want the same code to run regardless of whether users are in a Kaggle notebook or outside.

lucyhe added 9 commits December 12, 2023 23:50

Add --force flag for model downloads

6849e91

rename and refactor

bfd1d8c

comments and finish renaming

691b228

lint

1250c8c

fix is_supported and lint

8e863e4

new tests in test_cache.py

eb3addf

lint

6f03be0

add more unit tests

132e21d

comment

b168200

lucyhe commented Dec 14, 2023

View reviewed changes

lucyhe requested a review from rosbo December 14, 2023 01:33

rosbo approved these changes Dec 14, 2023

View reviewed changes

check for deletes in test_cache.py

d27fbf6

rosbo reviewed Dec 14, 2023

View reviewed changes

lucyhe added 4 commits December 14, 2023 22:33

more assertions and a new test

20303a1

os.rmdir instead of os.removedirs

5ba20ee

comment

75cdf21

remove redundant if

f384091

rosbo reviewed Dec 15, 2023

View reviewed changes

Log when force_download is used in kaggle cache, rather than raise error

70f6616

lucyhe merged commit 8345bb3 into main Dec 15, 2023
6 checks passed

lucyhe deleted the lh/cache branch December 15, 2023 01:16

lucyhe changed the title ~~Implement force_download option for model_download over HTTP~~ [b/307966230] Implement force_download option for model_download over HTTP Dec 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[b/307966230] Implement force_download option for model_download over HTTP #44

[b/307966230] Implement force_download option for model_download over HTTP #44

lucyhe commented Dec 14, 2023 •

edited

Loading

lucyhe Dec 14, 2023

rosbo Dec 14, 2023

lucyhe Dec 14, 2023

rosbo Dec 14, 2023

lucyhe commented Dec 14, 2023

rosbo left a comment

rosbo Dec 14, 2023

rosbo Dec 14, 2023

rosbo Dec 14, 2023

rosbo Dec 14, 2023

rosbo Dec 14, 2023

lucyhe Dec 15, 2023

rosbo Dec 14, 2023

rosbo commented Dec 14, 2023

lucyhe commented Dec 14, 2023

rosbo Dec 14, 2023 •

edited

Loading

lucyhe Dec 14, 2023

rosbo Dec 14, 2023

rosbo Dec 15, 2023


		deleted_path = delete_from_cache(TEST_MODEL_HANDLE, path=TEST_FILEPATH)

		self.assertEqual(

	def create_test_cache():
	with TemporaryDirectory() as d:
	with mock.patch.dict(os.environ, {CACHE_FOLDER_ENV_VAR_NAME: d}):
	yield d

[b/307966230] Implement force_download option for model_download over HTTP #44

[b/307966230] Implement force_download option for model_download over HTTP #44

Conversation

lucyhe commented Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyhe commented Dec 14, 2023

rosbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosbo commented Dec 14, 2023

lucyhe commented Dec 14, 2023

rosbo Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyhe commented Dec 14, 2023 •

edited

Loading

rosbo Dec 14, 2023 •

edited

Loading