Repository: huggingface/huggingface_hub
Pull request #2142 (2024-03-22 10:51:06)

ModelHubMixin overwrite config if preexistant
Good catch from @not-lain. If a config file is saved by `_save_pretrained`, we don't want to overwrite it (done in https://github.com/huggingface/huggingface_hub/issues/2102. But if a config file already exists before saving, we should overwrite it (as any other file). This PR fixes this + adds a test case.

(also removing a warning in a test)

HuggingFaceDocBuilderDev: The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_2142). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.
codecov-commenter: ## [Codecov](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/2142?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 82.69%. Comparing base [(`3252e27`)](https://app.codecov.io/gh/huggingface/huggingface_hub/commit/3252e27ac86ceac9485146a324686a39322d4bd2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) to head [(`e248ada`)](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/2142?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface).


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #2142      +/-   ##
==========================================
- Coverage   82.70%   82.69%   -0.01%     
==========================================
  Files         103      103              
  Lines        9628     9629       +1     
==========================================
  Hits         7963     7963              
- Misses       1665     1666       +1     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/2142?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface).


Patch:
diff --git a/src/huggingface_hub/hub_mixin.py b/src/huggingface_hub/hub_mixin.py
index 86dce2dd..fa9ee5e7 100644
--- a/src/huggingface_hub/hub_mixin.py
+++ b/src/huggingface_hub/hub_mixin.py
@@ -262,6 +262,12 @@ class ModelHubMixin:
         save_directory = Path(save_directory)
         save_directory.mkdir(parents=True, exist_ok=True)
 
+        # Remove config.json if already exists. After `_save_pretrained` we don't want to overwrite config.json
+        # as it might have been saved by the custom `_save_pretrained` already. However we do want to overwrite
+        # an existing config.json if it was not saved by `_save_pretrained`.
+        config_path = save_directory / CONFIG_NAME
+        config_path.unlink(missing_ok=True)
+
         # save model weights/files (framework-specific)
         self._save_pretrained(save_directory)
 
@@ -271,7 +277,6 @@ class ModelHubMixin:
         if config is not None:
             if is_dataclass(config):
                 config = asdict(config)  # type: ignore[arg-type]
-            config_path = save_directory / CONFIG_NAME
             if not config_path.exists():
                 config_str = json.dumps(config, sort_keys=True, indent=2)
                 config_path.write_text(config_str)


Test patch:
diff --git a/tests/test_hub_mixin.py b/tests/test_hub_mixin.py
index ed137c2f..09dd91c5 100644
--- a/tests/test_hub_mixin.py
+++ b/tests/test_hub_mixin.py
@@ -302,10 +302,29 @@ class HubMixinTest(unittest.TestCase):
         # Delete repo
         self._api.delete_repo(repo_id=repo_id)
 
-    def test_save_pretrained_do_not_overwrite_config(self):
-        """Regression test for https://github.com/huggingface/huggingface_hub/issues/2102."""
+    def test_save_pretrained_do_not_overwrite_new_config(self):
+        """Regression test for https://github.com/huggingface/huggingface_hub/issues/2102.
+
+        If `_from_pretrained` does save a config file, we should not overwrite it.
+        """
         model = DummyModelSavingConfig()
         model.save_pretrained(self.cache_dir)
         # config.json is not overwritten
         with open(self.cache_dir / "config.json") as f:
             assert json.load(f) == {"custom_config": "custom_config"}
+
+    def test_save_pretrained_does_overwrite_legacy_config(self):
+        """Regression test for https://github.com/huggingface/huggingface_hub/issues/2142.
+
+        If a previously existing config file exists, it should be overwritten.
+        """
+        # Something existing in the cache dir
+        (self.cache_dir / "config.json").write_text(json.dumps({"something_legacy": 123}))
+
+        # Save model
+        model = DummyModelWithKwargs(a=1, b=2)
+        model.save_pretrained(self.cache_dir)
+
+        # config.json IS overwritten
+        with open(self.cache_dir / "config.json") as f:
+            assert json.load(f) == {"a": 1, "b": 2}
diff --git a/tests/test_hub_mixin_pytorch.py b/tests/test_hub_mixin_pytorch.py
index 48511b08..317b0a00 100644
--- a/tests/test_hub_mixin_pytorch.py
+++ b/tests/test_hub_mixin_pytorch.py
@@ -346,10 +346,10 @@ class PytorchHubMixinTest(unittest.TestCase):
 
         # Linear layers should share weights and biases in memory
         state_dict = reloaded.state_dict()
-        a_weight_ptr = state_dict["a.weight"].storage().data_ptr()
-        b_weight_ptr = state_dict["b.weight"].storage().data_ptr()
-        a_bias_ptr = state_dict["a.bias"].storage().data_ptr()
-        b_bias_ptr = state_dict["b.bias"].storage().data_ptr()
+        a_weight_ptr = state_dict["a.weight"].untyped_storage().data_ptr()
+        b_weight_ptr = state_dict["b.weight"].untyped_storage().data_ptr()
+        a_bias_ptr = state_dict["a.bias"].untyped_storage().data_ptr()
+        b_bias_ptr = state_dict["b.bias"].untyped_storage().data_ptr()
         assert a_weight_ptr == b_weight_ptr
         assert a_bias_ptr == b_bias_ptr