Fix timezone aware datetimes backwards compatibility #72

mraspaud · 2025-01-29T19:38:07Z

This PR fixes some backwards incompatibility, in particular in the name server, so that replies are the same message version as the request.

Closes #71

adybbroe

Great work, nice refactoring also. And the tests help a lot understanding what is going on.
It seems to do what it shall - I have tested the nameserver in development at SMHI and seems like it supports both new and old (without timezone) messages - so indeed backward compatible. Will let it run over the night and see if it continues to behave well.

Would be good if someone else could review this PR also, as I have very little insight in this library.

codecov · 2025-01-31T10:29:03Z

Codecov Report

Attention: Patch coverage is 86.75958% with 38 lines in your changes missing coverage. Please review.

Project coverage is 89.42%. Comparing base (87962f3) to head (c032ad3).
Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
posttroll/ns.py	39.21%	31 Missing ⚠️
posttroll/address_receiver.py	86.20%	4 Missing ⚠️
posttroll/backends/zmq/subscriber.py	50.00%	2 Missing ⚠️
posttroll/message_broadcaster.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
- Coverage   89.59%   89.42%   -0.18%     
==========================================
  Files          25       25              
  Lines        2278     2411     +133     
==========================================
+ Hits         2041     2156     +115     
- Misses        237      255      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mraspaud · 2025-02-03T11:51:06Z

@pnuu I think this pr is finally ready for you review. It has grown much bigger than what I would have expected/wanted, but I found quite a few bugs that I couldn’t leave in the code. Hopefully the tests are now also more isolated/reliable, even there is still some work left in that domain.

pnuu

In general I approve the changes. Some comments inline.

I'm really hopeless with type hinting, so I got confused about the foo:list[str] = [] syntax. The syntax in the function declarations seem to be different that I've seen in Satpy and in internet. Are the added type hints checked by mypy, and are they crucial where they are added? There are function declarations that were updated but the type hints were not added, and others where the hints are added only to some arguments but not all.

pnuu · 2025-02-03T11:53:53Z

posttroll/address_receiver.py

    return config.get("address_publish_port", DEFAULT_ADDRESS_PUBLISH_PORT)


 def get_local_ips():
    """Get local IP addresses."""
    inet_addrs = [netifaces.ifaddresses(iface).get(netifaces.AF_INET)
                  for iface in netifaces.interfaces()]
-    ips = []
+    ips:list[str] = []


What does this do? I don't undersand the syntax.

Oh, a type hint. I think there should be a space: ips: list[str] = [] 🤔

addressed in 18b6c1a

pnuu · 2025-02-03T11:55:32Z

posttroll/address_receiver.py

@@ -204,7 +204,7 @@ def set_up_address_receiver(self, port):
        """Set up the address receiver depending on if it is multicast or not."""
        nameservers = False
        if self._multicast_enabled:
-            while True:
+            while True:  # should this really be tried forever? wouldn't it be enough with 3?


I agree. If it keeps on failing, better fail so that it is visible.

addressed in 18b6c1a

pnuu · 2025-02-03T12:04:19Z

posttroll/backends/zmq/ns.py

@@ -15,39 +16,55 @@
 nslock = Lock()


-def zmq_get_pub_address(name, timeout=10, nameserver="localhost"):
+def zmq_get_pub_address(name:str, timeout:float|int=10, nameserver:str="localhost"):


I'm not at all familiar with type hinting syntax, but this looks different to the examples I see on the net or in Satpy

whitespace between the variable and type name: str

whitespace between possible types and kwarg default values: timetout: float | int = 1

addressed in 18b6c1a

pnuu · 2025-02-03T12:07:28Z

posttroll/backends/zmq/ns.py

+        self.running:bool = True
+        self.listener:SocketReceiver|None = None


Here the typehints should have spaces added for clarity.

addressed in 18b6c1a

pnuu · 2025-02-03T12:20:53Z

posttroll/ns.py


    def stop(self):
        """Stop the nameserver."""
        return self._ns.stop()
+
+
+def main():


Could the runnable script stay outside of the library? Or is there a benefit of moving it within?

yes, we can then have it as a script in pyproject.toml if it’s defined in the library

Right, so the scripts actually can't be outside of the package with pyproject.toml 🙈 I've liked to have them outside so that they don't affect the test coverage 😅

mraspaud · 2025-02-03T15:53:54Z

Regarding type hints: I got basedpyright running in my editor, and the posttroll code lit up like a Christmas tree. Hence my starting to fix a few of the issues. I then found out how to dim the lights a bit so that only the errors would show, and then I stopped adding type hints. I could remove them, but I thought it was a step in the right direction.

pnuu · 2025-02-04T07:35:06Z

doc/source/index.rst

+Handling timezone-aware datetime objects
+----------------------------------------
+
+Timezone-aware datetime object were historically unsupported in posttroll, such as encoding or decoding them was
+leading to problems. Recent versions of posttroll were fixed to address the problem, however message sent with these
+versions are not backwards compatible. To ensure backwards compatibility, it is possible to configure posttroll to send
+message that drop the timezone information on encoding. This can be done with an environment variable::
+
+  POSTTROLL_MESSAGE_VERSION=v1.01
+
+or within python code::
+
+   >>> from posttroll import config
+   >>> with config.set(message_version="v1.01"):
+   ...
+


Reading this it is possible to get the impression that also the datetimes within the message data could be affected. But as far as I've understood the code changes, it is actually only the header portion (the message creation time) that is affected and the times within the message data come from what ever created them.

No, the metadata is also affected. That’s actually how we noticed the problem in the first place, as an older version of posttroll was refusing to decode the start_time item of the metadata and was leaving it as a string.

Ok, got it. When receiving messages with tz aware datetimes, there's a problem decoding the messages. I didn't see this in my tests because the crash happened already with the message creation time.

So the processes earlier in the chain can't use tz aware datetimes within the message data unless the later parts have the fix. So the later parts will need the fix first, so they will already handle both aware and unaware datetimes. Nameserver only needs to understand the message creation time, as it doesn't need to actually parse the messages.

Did I get that right or did I miss something?

That’s correct. Nameserver messages don’t have any encoded time in their body (it’s just text iirc).

mraspaud added 3 commits January 29, 2025 20:08

Fix timezone aware datetimes and some backwards compatibility

2ea5e67

Make sure nameserver is backwards compatible

dddeb76

Merge branch 'main' into backwards_compat

6e31841

mraspaud requested review from adybbroe and pnuu January 29, 2025 19:38

mraspaud self-assigned this Jan 29, 2025

mraspaud added the bug label Jan 29, 2025

mraspaud added 5 commits January 29, 2025 20:42

Fix merge bug

82831a8

Fix test

6f4de41

Fix style

696ff04

Include nameserver in scripts

9fbdcf2

Remove daemon variant of the nameserver script

5bcac12

mraspaud marked this pull request as ready for review January 30, 2025 09:53

adybbroe approved these changes Jan 30, 2025

View reviewed changes

mraspaud added 2 commits January 31, 2025 10:41

Add some type hints

2c2c701

Fix address receiver bug

10dddec

mraspaud added 10 commits January 31, 2025 13:34

Cleanup

dbb3d54

Make authentication procedure and tests more robust

a93020c

Ensure separate contexts for nameserver tests

ab3b186

Make random mc_groups deterministic

3156ca2

Fix some bugs

cc4dbc7

Add info log about mc group received

7c33bb5

Fix closing sockets on context termination

0c44181

Terminate context in secure zmq tests

e6bb27f

Fix typo and logging

490e77e

Clean up

010be39

pnuu reviewed Feb 3, 2025

View reviewed changes

Fix race condition in authentication

d336876

Fix reviewer comments

18b6c1a

Add some documentation and cleanup

c032ad3

pnuu reviewed Feb 4, 2025

View reviewed changes

mraspaud merged commit 2fde7e5 into pytroll:main Feb 4, 2025
6 of 7 checks passed

mraspaud deleted the backwards_compat branch February 4, 2025 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix timezone aware datetimes backwards compatibility #72

Fix timezone aware datetimes backwards compatibility #72

mraspaud commented Jan 29, 2025 •

edited

Loading

adybbroe left a comment

codecov bot commented Jan 31, 2025 •

edited

Loading

mraspaud commented Feb 3, 2025

pnuu left a comment

pnuu Feb 3, 2025

pnuu Feb 3, 2025

mraspaud Feb 3, 2025

pnuu Feb 3, 2025

mraspaud Feb 3, 2025

pnuu Feb 3, 2025

mraspaud Feb 3, 2025

pnuu Feb 3, 2025

mraspaud Feb 3, 2025

pnuu Feb 3, 2025

mraspaud Feb 3, 2025

pnuu Feb 4, 2025

mraspaud commented Feb 3, 2025

pnuu Feb 4, 2025

mraspaud Feb 4, 2025

pnuu Feb 4, 2025

mraspaud Feb 4, 2025

		self.running:bool = True
		self.listener:SocketReceiver\|None = None

Fix timezone aware datetimes backwards compatibility #72

Fix timezone aware datetimes backwards compatibility #72

Conversation

mraspaud commented Jan 29, 2025 • edited Loading

adybbroe left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 31, 2025 • edited Loading

Codecov Report

mraspaud commented Feb 3, 2025

pnuu 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

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

mraspaud commented Feb 3, 2025

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

mraspaud commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 31, 2025 •

edited

Loading