Repository: huggingface/huggingface_hub
Pull request #999 (2022-08-19 10:00:55)

Add an alias that makes dateutil optional.
`python-dateutil` usage has been introduced by the Community API PR https://github.com/huggingface/huggingface_hub/pull/930. However, this adds an extra dependency to `huggingface_hub` which we want to avoid when possible. The dependency has not been formerly added to `setup.py` which was reported [in this thread](https://huggingface.slack.com/archives/C02V5EA0A95/p1660839367099929?thread_ts=1660827165.997349&cid=C02V5EA0A95) (internal link) when doing the 0.9 pre-release.

Since `dateutil` is only used to parse a UTC-isoformatted date string returned from the server, we can assume that we know which format to use to parse the date string. Given that, it is possible to build a weak parser using a pure python implementation.

That said, I am still quite undecided on how to proceed. I see multiple solutions:
1. Add `python-dateutil` as a dependency. It's straightforward but my understanding is we want to avoid 3rd-party libraries when only used for a "simple" use case.
2. Have a weak parser expecting `"%Y-%m-%dT%H:%M:%S.%fZ"` format. This works without any third-party dependency. Big drawback is that we are dependent on any change on the server. My expectation is that "it shouldn't happen" but if it happens it means we would need to do a hot-fix on all older versions of `huggingface_hub`.
3. The in-between solution: by default, `parse_datetime` will use `dateutil` if it's installed. Otherwise, it will default to the weak parser. In the future if the server response format changes, a `ValueError` will be raised with message `Please run "pip install huggingface_hub[dateutil]"` which seems less problematic than hot-fixing `huggingface_hub`. Drawback here is that I found it shaky to have a different parser depending on the installation.


For now I implemented solution 3. but I'd be very happy to know your opinion on it. If solution 1 or 2 or anything else is preferable, I don't mind changing it.

Ping @LysandreJik @adrinjalali @osanseviero since I guess you have more experience/are more opinionated on how to manage an open-source project. Ping also @SBrandeis @Pierrci working on moon-landing.



codecov[bot]: # [Codecov](https://codecov.io/gh/huggingface/huggingface_hub/pull/999?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) Report
> Merging [#999](https://codecov.io/gh/huggingface/huggingface_hub/pull/999?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) (48d58fd) into [main](https://codecov.io/gh/huggingface/huggingface_hub/commit/f12a61ea50bb8764695decd07a100bdb4227f7e4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) (f12a61e) will **decrease** coverage by `31.12%`.
> The diff coverage is `42.85%`.

```diff
@@             Coverage Diff             @@
##             main     #999       +/-   ##
===========================================
- Coverage   81.38%   50.26%   -31.13%     
===========================================
  Files          30       31        +1     
  Lines        3401     3426       +25     
===========================================
- Hits         2768     1722     -1046     
- Misses        633     1704     +1071     
```


| [Impacted Files](https://codecov.io/gh/huggingface/huggingface_hub/pull/999?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) | Coverage Δ | |
|---|---|---|
| [src/huggingface\_hub/utils/\_datetime.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi91dGlscy9fZGF0ZXRpbWUucHk=) | `36.00% <36.00%> (ø)` | |
| [src/huggingface\_hub/community.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9jb21tdW5pdHkucHk=) | `75.00% <100.00%> (-16.67%)` | :arrow_down: |
| [src/huggingface\_hub/hf\_api.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9oZl9hcGkucHk=) | `38.07% <100.00%> (-48.97%)` | :arrow_down: |
| [src/huggingface\_hub/utils/\_\_init\_\_.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi91dGlscy9fX2luaXRfXy5weQ==) | `100.00% <100.00%> (ø)` | |
| [src/huggingface\_hub/snapshot\_download.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9zbmFwc2hvdF9kb3dubG9hZC5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [src/huggingface\_hub/repocard.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9yZXBvY2FyZC5weQ==) | `23.86% <0.00%> (-71.60%)` | :arrow_down: |
| [src/huggingface\_hub/\_snapshot\_download.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9fc25hcHNob3RfZG93bmxvYWQucHk=) | `19.64% <0.00%> (-71.43%)` | :arrow_down: |
| [src/huggingface\_hub/keras\_mixin.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9rZXJhc19taXhpbi5weQ==) | `19.64% <0.00%> (-70.84%)` | :arrow_down: |
| [src/huggingface\_hub/inference\_api.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9pbmZlcmVuY2VfYXBpLnB5) | `32.35% <0.00%> (-55.89%)` | :arrow_down: |
| [src/huggingface\_hub/utils/endpoint\_helpers.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi91dGlscy9lbmRwb2ludF9oZWxwZXJzLnB5) | `44.08% <0.00%> (-53.77%)` | :arrow_down: |
| ... and [13 more](https://codecov.io/gh/huggingface/huggingface_hub/pull/999/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface)

HuggingFaceDocBuilderDev: The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_999). All of your documentation changes will be reflected on that endpoint.
adrinjalali: yeah I'm not sure which one's the best option. I'd rather do either option 1 or 3. Not sure which one I prefer.

(you can also make codecov to wait for all tests before publishing the results by telling it how many updates it should wait for)
Wauplin: > yeah I'm not sure which one's the best option. I'd rather do either option 1 or 3. Not sure which one I prefer.

Thanks for the quick feedback. Let's see what others think, both 1. and 3. sound "not so good" and "not so bad" to me.

> (you can also make codecov to wait for all tests before publishing the results by telling it how many updates it should wait for)

Thanks for the tip, I should definitely look into this otherwise I'm already annoyed by it 🙄 
osanseviero: Given this is a simple format, I'm inclined towards option 2. Server format should not change if we want to keep backwards compatibility. WDYT @SBrandeis?

Patch:
diff --git a/Makefile b/Makefile
index 4d6ac076..97d32743 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
 .PHONY: quality style test
 
 
-check_dirs := tests src
+check_dirs := tests src setup.py
 
 
 quality:
diff --git a/codecov.yml b/codecov.yml
new file mode 100644
index 00000000..a92282aa
--- /dev/null
+++ b/codecov.yml
@@ -0,0 +1,5 @@
+comment:
+  # https://docs.codecov.com/docs/pull-request-comments#requiring-changes
+  require_changes: true
+  # https://docs.codecov.com/docs/pull-request-comments#after_n_builds
+  after_n_builds: 11
\ No newline at end of file
diff --git a/src/huggingface_hub/community.py b/src/huggingface_hub/community.py
index 57ee34e4..90afc486 100644
--- a/src/huggingface_hub/community.py
+++ b/src/huggingface_hub/community.py
@@ -9,7 +9,7 @@ from dataclasses import dataclass
 from datetime import datetime
 from typing import List, Optional
 
-from dateutil.parser import parse as parse_datetime
+from .utils import parse_datetime
 
 
 if sys.version_info >= (3, 8):
diff --git a/src/huggingface_hub/fastai_utils.py b/src/huggingface_hub/fastai_utils.py
index 7fee0f6c..0eba020d 100644
--- a/src/huggingface_hub/fastai_utils.py
+++ b/src/huggingface_hub/fastai_utils.py
@@ -353,7 +353,7 @@ def push_to_hub_fastai(
     learner,
     repo_id: str,
     commit_message: Optional[str] = "Add model",
-    private: Optional[bool] = None,
+    private: bool = False,
     token: Optional[str] = None,
     config: Optional[dict] = None,
     **kwargs,
@@ -369,7 +369,7 @@ def push_to_hub_fastai(
             The repository id for your model in Hub in the format of "namespace/repo_name". The namespace can be your individual account or an organization to which you have write access (for example, 'stanfordnlp/stanza-de').
         commit_message (`str`, *optional*):
             Message to commit while pushing. Will default to :obj:`"add model"`.
-        private (`bool`, *optional*):
+        private (`bool`, *optional*, defaults to `False`):
             Whether or not the repository created should be private.
         token (`str`, *optional*):
             The Hugging Face account token to use as HTTP bearer authorization for remote files. If :obj:`None`, the token will be asked by a prompt.
diff --git a/src/huggingface_hub/hf_api.py b/src/huggingface_hub/hf_api.py
index 6e324073..8e6233e6 100644
--- a/src/huggingface_hub/hf_api.py
+++ b/src/huggingface_hub/hf_api.py
@@ -22,7 +22,6 @@ from typing import BinaryIO, Dict, Iterable, Iterator, List, Optional, Tuple, Un
 from urllib.parse import quote
 
 import requests
-from dateutil.parser import parse as parse_datetime
 from requests.exceptions import HTTPError
 
 from ._commit_api import (
@@ -51,7 +50,7 @@ from .constants import (
     REPO_TYPES_URL_PREFIXES,
     SPACES_SDK_TYPES,
 )
-from .utils import filter_repo_objects, logging
+from .utils import filter_repo_objects, logging, parse_datetime
 from .utils._deprecation import _deprecate_positional_args
 from .utils._errors import (
     _raise_convert_bad_request,
@@ -237,7 +236,7 @@ class ModelInfo:
             Pipeline tag to identify the correct widget.
         siblings (`List[RepoFile]`, *optional*):
             list of ([`huggingface_hub.hf_api.RepoFile`]) objects that constitute the model.
-        private (`bool`, *optional*):
+        private (`bool`, *optional*, defaults to `False`):
             is the repo private
         author (`str`, *optional*):
             repo author
@@ -256,7 +255,7 @@ class ModelInfo:
         tags: Optional[List[str]] = None,
         pipeline_tag: Optional[str] = None,
         siblings: Optional[List[Dict]] = None,
-        private: Optional[bool] = None,
+        private: bool = False,
         author: Optional[str] = None,
         config: Optional[Dict] = None,
         **kwargs,
@@ -303,7 +302,7 @@ class DatasetInfo:
             List of tags.
         siblings (`List[RepoFile]`, *optional*):
             list of [`huggingface_hub.hf_api.RepoFile`] objects that constitute the dataset.
-        private (`bool`, *optional*):
+        private (`bool`, *optional*, defaults to `False`):
             is the repo private
         author (`str`, *optional*):
             repo author
@@ -325,7 +324,7 @@ class DatasetInfo:
         lastModified: Optional[str] = None,
         tags: Optional[List[str]] = None,
         siblings: Optional[List[Dict]] = None,
-        private: Optional[bool] = None,
+        private: bool = False,
         author: Optional[str] = None,
         description: Optional[str] = None,
         citation: Optional[str] = None,
@@ -378,7 +377,7 @@ class SpaceInfo:
             date of last commit to repo
         siblings (`List[RepoFile]`, *optional*):
             list of [`huggingface_hub.hf_api.RepoFIle`] objects that constitute the Space
-        private (`bool`, *optional*):
+        private (`bool`, *optional*, defaults to `False`):
             is the repo private
         author (`str`, *optional*):
             repo author
@@ -393,7 +392,7 @@ class SpaceInfo:
         sha: Optional[str] = None,
         lastModified: Optional[str] = None,
         siblings: Optional[List[Dict]] = None,
-        private: Optional[bool] = None,
+        private: bool = False,
         author: Optional[str] = None,
         **kwargs,
     ):
@@ -1525,7 +1524,7 @@ class HfApi:
         *,
         token: Optional[str] = None,
         organization: Optional[str] = None,
-        private: Optional[bool] = None,
+        private: bool = False,
         repo_type: Optional[str] = None,
         exist_ok: Optional[bool] = False,
         space_sdk: Optional[str] = None,
@@ -1546,7 +1545,7 @@ class HfApi:
 
             token (`str`, *optional*):
                 An authentication token (See https://huggingface.co/settings/token)
-            private (`bool`, *optional*):
+            private (`bool`, *optional*, defaults to `False`):
                 Whether the model repo should be private.
             repo_type (`str`, *optional*):
                 Set to `"dataset"` or `"space"` if uploading to a dataset or
diff --git a/src/huggingface_hub/hub_mixin.py b/src/huggingface_hub/hub_mixin.py
index 757e98c5..a90cd2b4 100644
--- a/src/huggingface_hub/hub_mixin.py
+++ b/src/huggingface_hub/hub_mixin.py
@@ -256,7 +256,7 @@ class ModelHubMixin:
         repo_url: Optional[str] = None,
         commit_message: Optional[str] = "Add model",
         organization: Optional[str] = None,
-        private: Optional[bool] = None,
+        private: bool = False,
         api_endpoint: Optional[str] = None,
         use_auth_token: Optional[Union[bool, str]] = None,
         git_user: Optional[str] = None,
@@ -274,7 +274,7 @@ class ModelHubMixin:
         # repo_id: str,
         # *,
         # commit_message: Optional[str] = "Add model",
-        # private: Optional[bool] = None,
+        # private: bool = False,
         # api_endpoint: Optional[str] = None,
         # token: Optional[str] = None,
         # branch: Optional[str] = None,
@@ -294,7 +294,7 @@ class ModelHubMixin:
                 Repository name to which push.
             commit_message (`str`, *optional*):
                 Message to commit while pushing.
-            private (`bool`, *optional*):
+            private (`bool`, *optional*, defaults to `False`):
                 Whether the repository created should be private.
             api_endpoint (`str`, *optional*):
                 The API endpoint to use when pushing the model to the hub.
diff --git a/src/huggingface_hub/keras_mixin.py b/src/huggingface_hub/keras_mixin.py
index eb1f1cac..04141b1a 100644
--- a/src/huggingface_hub/keras_mixin.py
+++ b/src/huggingface_hub/keras_mixin.py
@@ -315,7 +315,7 @@ def push_to_hub_keras(
     log_dir: Optional[str] = None,
     commit_message: Optional[str] = "Add model",
     organization: Optional[str] = None,
-    private: Optional[bool] = None,
+    private: bool = False,
     api_endpoint: Optional[str] = None,
     use_auth_token: Optional[Union[bool, str]] = True,
     git_user: Optional[str] = None,
@@ -337,7 +337,7 @@ def push_to_hub_keras(
     # repo_id: str,
     # *,
     # commit_message: Optional[str] = "Add model",
-    # private: Optional[bool] = None,
+    # private: bool = None,
     # api_endpoint: Optional[str] = None,
     # token: Optional[str] = True,
     # branch: Optional[str] = None,
@@ -367,7 +367,7 @@ def push_to_hub_keras(
             Repository name to which push
         commit_message (`str`, *optional*, defaults to "Add message"):
             Message to commit while pushing.
-        private (`bool`, *optional*):
+        private (`bool`, *optional*, defaults to `False`):
             Whether the repository created should be private.
         api_endpoint (`str`, *optional*):
             The API endpoint to use when pushing the model to the hub.
diff --git a/src/huggingface_hub/utils/__init__.py b/src/huggingface_hub/utils/__init__.py
index 9267b25d..f49c87be 100644
--- a/src/huggingface_hub/utils/__init__.py
+++ b/src/huggingface_hub/utils/__init__.py
@@ -16,6 +16,7 @@
 # limitations under the License
 
 from . import tqdm as _tqdm  # _tqdm is the module
+from ._datetime import parse_datetime
 from ._errors import (
     EntryNotFoundError,
     LocalEntryNotFoundError,
diff --git a/src/huggingface_hub/utils/_datetime.py b/src/huggingface_hub/utils/_datetime.py
new file mode 100644
index 00000000..3af2e1c7
--- /dev/null
+++ b/src/huggingface_hub/utils/_datetime.py
@@ -0,0 +1,65 @@
+# coding=utf-8
+# Copyright 2022-present, the HuggingFace Inc. team.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Contains utilities to handle datetimes in Huggingface Hub."""
+from datetime import datetime, timezone
+
+
+# Local machine offset compared to UTC
+# Taken from https://stackoverflow.com/a/3168394.
+UTC_OFFSET = datetime.now(timezone.utc).astimezone().utcoffset()
+
+
+def parse_datetime(date_string: str) -> datetime:
+    """
+    Parses a date_string returned from the server to a datetime object.
+
+    This parser is a weak-parser is the sense that it handles only a single format of
+    date_string. It is expected that the server format will never change. The
+    implementation depends only on the standard lib to avoid an external dependency
+    (python-dateutil). See full discussion about this decision on PR:
+    https://github.com/huggingface/huggingface_hub/pull/999.
+
+    Usage:
+        ```py
+        > parse_datetime('2022-08-19T07:19:38.123Z')
+        datetime.datetime(2022, 8, 19, 7, 19, 38, 123000, tzinfo=timezone.utc)
+        ```
+
+    Args:
+        date_string (`str`):
+            A string representing a datetime returned by the Hub server.
+            String is expected to follow '%Y-%m-%dT%H:%M:%S.%fZ' pattern.
+
+    Returns:
+        A python datetime object.
+
+    Raises:
+        :class:`ValueError`:
+            If `date_string` cannot be parsed.
+    """
+    try:
+        # Datetime ending with a Z means "UTC". Here we parse the date as local machine
+        # timezone and then move it to the appropriate UTC timezone.
+        # See https://en.wikipedia.org/wiki/ISO_8601#Coordinated_Universal_Time_(UTC)
+        # Taken from https://stackoverflow.com/a/3168394.
+
+        dt = datetime.strptime(date_string, "%Y-%m-%dT%H:%M:%S.%fZ")
+        dt += UTC_OFFSET  # By default, datetime is not timezoned -> move to UTC time
+        return dt.astimezone(timezone.utc)  # Set explicit timezone
+    except ValueError as e:
+        raise ValueError(
+            f"Cannot parse '{date_string}' as a datetime. Date string is expected to"
+            " follow '%Y-%m-%dT%H:%M:%S.%fZ' pattern."
+        ) from e


Test patch:
diff --git a/tests/test_utils_datetime.py b/tests/test_utils_datetime.py
new file mode 100644
index 00000000..4fb350a4
--- /dev/null
+++ b/tests/test_utils_datetime.py
@@ -0,0 +1,32 @@
+import unittest
+from datetime import datetime, timezone
+
+import pytest
+
+from huggingface_hub.utils import parse_datetime
+
+
+class TestDatetimeUtils(unittest.TestCase):
+    def test_parse_datetime(self):
+        """Test `parse_datetime` works correctly on datetimes returned by server."""
+        self.assertEqual(
+            parse_datetime("2022-08-19T07:19:38.123Z"),
+            datetime(2022, 8, 19, 7, 19, 38, 123000, tzinfo=timezone.utc),
+        )
+
+        with pytest.raises(
+            ValueError, match=r".*Cannot parse '2022-08-19T07:19:38' as a datetime.*"
+        ):
+            parse_datetime("2022-08-19T07:19:38")
+
+        with pytest.raises(
+            ValueError,
+            match=r".*Cannot parse '2022-08-19T07:19:38.123' as a datetime.*",
+        ):
+            parse_datetime("2022-08-19T07:19:38.123")
+
+        with pytest.raises(
+            ValueError,
+            match=r".*Cannot parse '2022-08-19 07:19:38.123Z\+6:00' as a datetime.*",
+        ):
+            parse_datetime("2022-08-19 07:19:38.123Z+6:00")