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 pyinotify with watchdog #185

Merged
merged 19 commits into from
Feb 27, 2024
Merged

Conversation

mraspaud
Copy link
Member

@mraspaud mraspaud commented Oct 12, 2023

This PR replaces pyinotify which has been unmaintained for a few years by watchdog

@mraspaud mraspaud requested a review from pnuu as a code owner October 12, 2023 14:12
Copy link
Member

@pnuu pnuu left a comment

Choose a reason for hiding this comment

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

Great job! The code also seem way simpler without the original inotify version. I'll try to test this tomorrow, but if I can't find the time, just merge.

trollmoves/server.py Show resolved Hide resolved
@pnuu
Copy link
Member

pnuu commented Feb 20, 2024

These are the failures I see locally some times, but not with every run:

FAILED trollmoves/tests/functional/test_move_it_server_client.py::test_simple_publishing_with_untarring - KeyError: 'dataset'
ERROR trollmoves/tests/functional/test_move_it_server_client.py::test_simple_transfer - AttributeError: 'NoisyPublisher' object has no attribute 'heartbeat'
FAILED trollmoves/tests/test_server.py::test_file_detected_with_inotify_is_published - assert 0 == 1

I think the last one is solved by adding a short sleep between starting the MoveItServer and writing the file. I'll run the test in a loop for a while and if I don't get any failures with the added sleep I'll push that.

@pnuu
Copy link
Member

pnuu commented Feb 20, 2024

I can't figure out why the AttributeError: 'NoisyPublisher' object has no attribute 'heartbeat is seen in the CI. I've gotten this only very rarely locally.

The KeyError came because of subscribing to topics "" so also the heartbeat messages were received. ee6a783 is just a partial fix, as instead I often get AttributeError: 'NoneType' object has no attribute 'data' because there's no message within the timeout.

@pnuu
Copy link
Member

pnuu commented Feb 23, 2024

Changing the Client config in the tests so that a non-noisy publisher is created (publish_port = 40000 and nameserver = False) the heartbeat problem disappears completely.

@pnuu
Copy link
Member

pnuu commented Feb 23, 2024

The NoisyPublisher class in Posttroll does not have a heartbeat() method. It might be a fluke that sometimes that error is not seen. NoisyPublisher._publisher.heartbeat() exists, so I think we need to add a .heartbeat() method that calls `self._publisher.heartbeat(). Creating a PR and linking it here.

@pnuu
Copy link
Member

pnuu commented Feb 23, 2024

The heartbeat issue is fixed after we have merged pytroll/posttroll#63 and released a new version of Posttroll.

As a workaround we could switch the tests here to use non-noisy publishing.

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 90.30471% with 35 lines in your changes are missing coverage. Please review.

Project coverage is 91.08%. Comparing base (6e77ed1) to head (7c50b7b).
Report is 2 commits behind head on main.

Files Patch % Lines
trollmoves/move_it.py 63.38% 26 Missing ⚠️
trollmoves/server.py 90.90% 6 Missing ⚠️
trollmoves/move_it_base.py 95.55% 2 Missing ⚠️
trollmoves/mirror.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
+ Coverage   90.98%   91.08%   +0.09%     
==========================================
  Files          24       26       +2     
  Lines        5247     5370     +123     
==========================================
+ Hits         4774     4891     +117     
- Misses        473      479       +6     
Flag Coverage Δ
unittests 91.08% <90.30%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@mraspaud mraspaud merged commit 010c0d4 into pytroll:main Feb 27, 2024
5 of 6 checks passed
@mraspaud mraspaud deleted the replace-inotify branch February 27, 2024 14:53
@mraspaud mraspaud self-assigned this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate away from pyinotify
3 participants