Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace dateutil.tz with ZoneInfo and tzlocal for better cross-platform compatibility. #40

Merged
merged 10 commits into from
Apr 22, 2024

Conversation

junhaoliao
Copy link
Contributor

@junhaoliao junhaoliao commented Apr 20, 2024

References

It was found on Windows that running below snippet would raise an exception of AttributeError.

Example code:

import logging
from pathlib import Path
from clp_logging.handlers import CLPFileHandler

clp_handler = CLPFileHandler(Path("example.clp.zst"))
logger = logging.getLogger(__name__)
logger.addHandler(clp_handler)
logger.warn("example warning")

Exception:

Traceback (most recent call last):
  File "R:\test-loglib\pythonProject\main.py", line 6, in <module>
    clp_handler = CLPFileHandler(Path("example.clp.zst"))
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Junhao\AppData\Roaming\Python\Python311\site-packages\clp_logging\handlers.py", line 794, in __init__
    super().__init__(
  File "C:\Users\Junhao\AppData\Roaming\Python\Python311\site-packages\clp_logging\handlers.py", line 726, in __init__
    self.timestamp_format, self.timezone = _init_timeinfo(timestamp_format, timezone)
                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Junhao\AppData\Roaming\Python\Python311\site-packages\clp_logging\handlers.py", line 64, in _init_timeinfo
    tzp: Path = Path.resolve(Path(tzf._filename))  # type: ignore
                                  ^^^^^^^^^^^^^
AttributeError: 'tzlocal' object has no attribute '_filename'

Further analysis found attribute _filename does not exist on the object returned by dateutil.tz.gettz() because the Windows implementation is different than the Linux one, where the _filename attribute does exist.

Description

  1. Replaced usage of dateutil.tz.gettz in clp_logging/readers.py to get time zone information with Python's built-in zoneinfo.ZoneInfo (with a fallback to backports.zoneinfo.ZoneInfo when the Python version is below 3.9).
  2. Replaced usage of dateutil.tz.gettz in clp_logging/handlers.py to get IANA current time zone name with third-party tzlocal library.
  3. Added backports.zoneinfo to project requirements in pyproject.toml for getting local time zone from IANA names.
  4. Added tzlocal to project requirements in pyproject.toml for getting local time zone name.

Validation performed

Run this snippet on Windows without observing any errors:

import logging
from pathlib import Path
from typing import List

from clp_logging.handlers import CLPFileHandler
from clp_logging.readers import CLPFileReader, Log

clp_handler = CLPFileHandler(Path("example.clp.zst"))
logger = logging.getLogger(__name__)
logger.addHandler(clp_handler)
logger.warning("example warning")

# create a list of all Log objects
log_objects: List[Log] = []
with CLPFileReader(Path("example.clp.zst")) as clp_reader:
    for log in clp_reader:
        log_objects.append(log)
    print(log)

Output:

2024-04-20 00:40:30.899-04:00 WARNING __main__ example warning

pyproject.toml Outdated Show resolved Hide resolved
@@ -186,7 +192,7 @@ def read_preamble(self) -> int:
# We do not use the timestamp pattern from the preamble as it may
# be from other languages and therefore incompatible.
# self.timestamp_format = self.metadata[METADATA_TIMESTAMP_PATTERN_KEY]
self.timezone = dateutil.tz.gettz(self.metadata[METADATA_TZ_ID_KEY])
self.timezone = ZoneInfo(self.metadata[METADATA_TZ_ID_KEY])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class ZoneInfo is an implementation of tzinfo abstracted class, shall we narrow down the type of self.timezone to ZoneInfo instead?
This means we should also update the type annotation in Log._decode

pyproject.toml Outdated
"typing-extensions >= 3.7.4",
"tzlocal >= 5.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, according to the document, this lib needs Python >= 3.8. We need to support Python3.7.
PyPI: https://pypi.org/project/tzlocal/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's drop it to 5.1 then. That seems to be the last version that supports Python 3.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tz = "/".join([tzp.parent.name, tzp.name])
else:
tz = "UTC"
tz = tzlocal.get_localzone_name()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the official doc I didn't see examples w.r.t. error handling. But from the implementation details it seems like it could raise an exception or return None (according to their comments). Shall we add a try block to catch any exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. In fact when I was looking for a Python 3.7 compatible version of the lib, I found this from their docs:

https://pypi.org/project/tzlocal/5.1/#:~:text=tzlocal%204.0%20also%20adds%20an%20official%20function%20get_localzone_name()%20to%20get%20only%20the%20timezone%20name%2C%20instead%20of%20a%20timezone%20object.%20On%20unix%2C%20it%20can%20raise%20an%20error%20if%20you%20don%E2%80%99t%20have%20a%20timezone%20name%20configured%2C%20where%20get_localzone()%20will%20succeed%2C%20so%20only%20use%20that%20if%20you%20need%20the%20timezone%20name.

tzlocal 4.0 also adds an official function get_localzone_name() to get only the timezone name, instead of a timezone object. On unix, it can raise an error if you don’t have a timezone name configured, where get_localzone() will succeed, so only use that if you need the timezone name.

Co-authored-by: Lin Zhihao <[email protected]>
pyproject.toml Outdated Show resolved Hide resolved
@LinZhihao-723
Copy link
Member

Can you resolve the conflict and try the latest workflow?

Copy link
Member

@LinZhihao-723 LinZhihao-723 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit msg suggestion:
Replace dateutil.tz with ZoneInfo and tzlocal for better cross-platform compatibility. (#40)

@junhaoliao junhaoliao changed the title Replace dateutil.tz with ZoneInfo and tzlocal. Replace dateutil.tz with ZoneInfo and tzlocal for better cross-platform compatibility. Apr 22, 2024
@junhaoliao junhaoliao merged commit 6d355d3 into y-scope:main Apr 22, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants