Restore time_sync module on macOS #75

ludwigschwardt · 2023-09-04T21:57:55Z

MacOS does not have the adjtimex system call. As a result we have stripped out the entire time_sync module on non-Linux platforms.

However, packages like katsdpcontroller need the ClockState enum without the rest. Instead of spreading the complications, restore the time_sync module with a fake adjtimex instead.

It returns the correct time but with artificially large errors in the absence of real values (and the status is always "OK").

[This was a leftover from the beginning of the year when I wanted to test katsdpcontroller on my laptop. Nearly forgot about it - it was lurking in my working copy for months 🙂 ]

MacOS does not have the `adjtimex` system call. As a result we have stripped out the entire `time_sync` module on non-Linux platforms. However, packages like katsdpcontroller need the `ClockState` enum without the rest. Instead of spreading the complications, restore the `time_sync` module with a fake `adjtimex` instead. It returns the correct time but with artificially large errors in the absence of real values (and the status is always "OK").

bmerry

I'm not wild about this approach, because it means the sensors will assert things that are just made up, rather than reporting that the values can't be measured. The only meaningful thing it returns is the current time, and that's not even reported as a sensor value. My preference would be

the dummy adjtimex() function raises NotImplementedError
TimeSyncUpdater catches NotImplementedError and responds by setting all the sensors to inactive.

Only a couple of tests would need to be disabled on MacOS. If you also added a test that mocked in the NotImplemented implementation and checked the response, you could get test coverage of that fallback on every OS.

src/aiokatcp/adjtimex.py

bmerry · 2023-09-05T07:50:29Z

src/aiokatcp/adjtimex.py

+    adjtimex = _libc.adjtimex  # noqa: F811
+    adjtimex.argtypes = [ctypes.POINTER(Timex)]  # type: ignore[attr-defined]
+    adjtimex.restype = ctypes.c_int  # type: ignore[attr-defined]
+    adjtimex.errcheck = _errcheck  # type: ignore[attr-defined]


I don't really like this approach of replacing a function, and clearly mypy doesn't either. I'd rather have _adjtimex_dummy and _adjtimex_real functions (or pick better names) and then either assign one of them to a adjtimex variable, or have an adjtimex function that dispatches to the right one (I'd favour the former just for efficiency).

Good point. I like the Bruce-mypy tag-team clothesline 🤼‍♂️ I wanted to generate an AI picture of the event but Dall-E failed me 😂

To be fair: mypy didn't like the assignment to attributes, which apparently did not bother it before (there was only one type: ignore on the last line, but now they all complained). It was actually flake8 with the issue [Redefinition of unused name from line n].

.github/workflows/test.yml

Instead of kludging adjtimex to produce fake data on non-Linux systems, rather raise `NotImplementedError` and set the relevant sensors to an inactive state in `TimeSyncUpdater`. Test both cases in the unit tests.

ludwigschwardt · 2023-09-08T21:18:21Z

Thanks! I've pushed a new version incorporating your suggestions.

I can't figure out the failing tests though. It fails on py311 on both Linux and macOS, and it seems to be unrelated to my fixes. In fact, if I rerun the tests on an older commit that previously passed, it now fails there too 🤔 It also doesn't fail on my laptop. Rerunning these tests several times doesn't help, so it's not just a random fluke.

It's complaining about a closed event loop in TestUnclosedClient.

bmerry · 2023-09-11T07:29:39Z

Thanks! I've pushed a new version incorporating your suggestions.

I can't figure out the failing tests though. It fails on py311 on both Linux and macOS, and it seems to be unrelated to my fixes. In fact, if I rerun the tests on an older commit that previously passed, it now fails there too 🤔 It also doesn't fail on my laptop. Rerunning these tests several times doesn't help, so it's not just a random fluke.

It's complaining about a closed event loop in TestUnclosedClient.

The Python 3.11 failures I think are caused by python/cpython#107650, which got backported to 3.11. We'll need to investigate to see if this is a regression in Python or something that we should be handling better.

bmerry · 2023-09-11T07:36:02Z

The Python 3.11 failures I think are caused by python/cpython#107650, which got backported to 3.11. We'll need to investigate to see if this is a regression in Python or something that we should be handling better.

I don't think that should hold up this PR, but we should investigate it before making a new release.

bmerry · 2023-09-11T08:22:41Z

I've opened NGC-1089 for the Python 3.11 issue.

src/aiokatcp/adjtimex.py

tests/test_time_sync.py

bmerry · 2023-09-11T07:14:19Z

tests/test_time_sync.py

+    ("expected_status",),
+    [
+        pytest.param(Sensor.Status.NOMINAL, marks=_linux),
+        pytest.param(Sensor.Status.INACTIVE, marks=_non_linux),


Rather than (or perhaps in addition to) doing this test only on non-Linux, I'd suggest doing it on all OSes by using mocking to replace adjtimex by _no_adjtimex, which allows the NotImplementedError handling to be tested on any OS.

I'm not getting away here :-) Done!

.github/workflows/test.yml

bmerry · 2023-09-11T13:38:30Z

Also don't forget to bump the year in the copyright notice.

coveralls · 2023-09-11T15:03:23Z

coverage: 93.408% (-0.1%) from 93.512% when pulling 120544c on restore-time-sync-on-macos into b59b113 on master.

@bmerry

This appeases mypy and removes some superfluous bits. Thanks @bmerry for the suggestions. Co-authored-by: Bruce Merry <1963944+bmerry@users.noreply.github.com>

Mock adjtimex to _no_adjtimex on all systems.

ludwigschwardt · 2023-09-11T16:15:12Z

OK, after many force-pushes I'm ready for another round 😅

bmerry · 2023-09-12T06:22:43Z

.gitignore

@@ -7,3 +7,6 @@
 src/aiokatcp.egg-info
 *.pyc
 __pycache__
+
+# macOS detritus
+.DS_Store


I feel like that should go into your global core.excludesfile rather than here, because it's specific to your development environment rather than specific to the repository.

Thanks, I did not know about that file... I merely cargo-culted some other repo's ignores (katpoint, etc). Much better idea!

bmerry · 2023-09-12T06:27:17Z

tests/test_time_sync.py

+def test_smoke(
+    sensors: SensorSet, updater: TimeSyncUpdater, expected_status: Sensor.Status
+) -> None:
+    """Test with real adjtimex on Linux to ensure it interacts cleanly with kernel."""


Not sure that docstring is accurate. It only covers one of the two parametrisations.

Come to think of it, I'm not sure parametrisation is really necessary, since they're a partition. You could just write

expected_status = Sensor.Status.NOMINAL if sys.platform == "linux" else Sensor.Status.INACTIVE

inside the test function.

Thanks, I missed the partition. I originally had tests for linux and darwin until I merged them.

bmerry · 2023-09-12T06:29:04Z

tests/test_time_sync.py

+    assert isinstance(sensors["ntp.state"].value, ClockState)
+    for sensor in sensors.values():
+        # Check that it actually got updated
+        assert sensor.status == Sensor.Status.INACTIVE


A lot of code duplication here. I'd suggest either:

Parametrise test_smoke with a boolean that says whether to mock, and conditionally use mocker inside the test; or

Factor out a helper function that does all the work of the test, and have the test functions call it.

I've gone for option 1 and lost 20 lines.

Ignores specific to the user environment is best put in the global `core.excludesfile`.

Optionally pretend that adjtimex is not there, and set the expected sensor status based on that and whether we are on Linux. No need for skipping tests anymore, since `test_smoke` will always run in some shape or form. This consolidates the smoke tests as well. Improve the documentation.

ludwigschwardt · 2023-09-12T10:16:53Z

Thanks for the suggestions! You can check again.

bmerry

LGTM. Go ahead and merge - there is a separate ticket for the Python 3.11 failures.

ludwigschwardt added 3 commits September 4, 2023 23:46

Fix timex struct attribute name

610a85d

Add links to man pages for timeval, timex structs

1fad69d

ludwigschwardt requested a review from bmerry September 4, 2023 21:58

It's safe to measure coverage on macOS again

7b4d381

bmerry requested changes Sep 5, 2023

View reviewed changes

Let adjtimex be unimplemented on macOS instead

37e5c33

Instead of kludging adjtimex to produce fake data on non-Linux systems, rather raise `NotImplementedError` and set the relevant sensors to an inactive state in `TimeSyncUpdater`. Test both cases in the unit tests.

ludwigschwardt requested a review from bmerry September 8, 2023 21:18

bmerry requested changes Sep 11, 2023

View reviewed changes

ludwigschwardt force-pushed the restore-time-sync-on-macos branch 2 times, most recently from f9f35d6 to 0fa9e13 Compare September 11, 2023 15:01

Apply suggestions from code review

883d30a

This appeases mypy and removes some superfluous bits. Thanks @bmerry for the suggestions. Co-authored-by: Bruce Merry <1963944+bmerry@users.noreply.github.com>

ludwigschwardt force-pushed the restore-time-sync-on-macos branch from 0fa9e13 to 883d30a Compare September 11, 2023 15:05

ludwigschwardt added 3 commits September 11, 2023 18:08

Check absent adjtimex handling on Linux too

658a350

Mock adjtimex to _no_adjtimex on all systems.

Ignore macOS detritus

79210b1

Bump the year in the copyright notice

5f608a3

ludwigschwardt force-pushed the restore-time-sync-on-macos branch from daafb59 to 5f608a3 Compare September 11, 2023 16:08

ludwigschwardt requested a review from bmerry September 11, 2023 16:15

bmerry requested changes Sep 12, 2023

View reviewed changes

ludwigschwardt added 2 commits September 12, 2023 10:54

Remove macOS detritus detritus

c71b896

Ignores specific to the user environment is best put in the global `core.excludesfile`.

ludwigschwardt requested a review from bmerry September 12, 2023 10:16

bmerry approved these changes Sep 13, 2023

View reviewed changes

ludwigschwardt merged commit 3984b77 into master Sep 13, 2023
16 of 22 checks passed

ludwigschwardt deleted the restore-time-sync-on-macos branch September 13, 2023 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore time_sync module on macOS #75

Restore time_sync module on macOS #75

ludwigschwardt commented Sep 4, 2023 •

edited

Loading

bmerry left a comment

bmerry Sep 5, 2023

ludwigschwardt Sep 5, 2023

ludwigschwardt Sep 5, 2023 •

edited

Loading

ludwigschwardt commented Sep 8, 2023

bmerry commented Sep 11, 2023

bmerry commented Sep 11, 2023

bmerry commented Sep 11, 2023

bmerry Sep 11, 2023

ludwigschwardt Sep 11, 2023

bmerry commented Sep 11, 2023

coveralls commented Sep 11, 2023 •

edited

Loading

ludwigschwardt commented Sep 11, 2023

bmerry Sep 12, 2023

ludwigschwardt Sep 12, 2023

bmerry Sep 12, 2023

ludwigschwardt Sep 12, 2023

bmerry Sep 12, 2023

ludwigschwardt Sep 12, 2023

ludwigschwardt commented Sep 12, 2023

bmerry left a comment

Restore time_sync module on macOS #75

Restore time_sync module on macOS #75

Conversation

ludwigschwardt commented Sep 4, 2023 • edited Loading

bmerry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludwigschwardt Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

ludwigschwardt commented Sep 8, 2023

bmerry commented Sep 11, 2023

bmerry commented Sep 11, 2023

bmerry commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmerry commented Sep 11, 2023

coveralls commented Sep 11, 2023 • edited Loading

ludwigschwardt commented Sep 11, 2023

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

ludwigschwardt commented Sep 12, 2023

bmerry left a comment

Choose a reason for hiding this comment

ludwigschwardt commented Sep 4, 2023 •

edited

Loading

ludwigschwardt Sep 5, 2023 •

edited

Loading

coveralls commented Sep 11, 2023 •

edited

Loading