Project

General

Profile

Actions

Idea #21146

closed

Replace ws4py dependency from PySDK

Added by Brett Smith 9 months ago. Updated 3 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Start date:
12/01/2023
Due date:
Story points:
-
Release:
Release relationship:
Auto

Description

The GitHub project says "[Project on Hiatus]"

The last commit just removed a similar warning from the README.

This is becoming a liability because the project build structure relies on a stack that's more and more deprecated. It's becoming harder to install on newer distributions and that problem is only going to get worse over time.

Find and switch to a replacement while maintaining the API of arvados.events and other affected modules.


Subtasks 1 (0 open1 closed)

Task #21242: Review 21146-pysdk-new-websocketsResolvedPeter Amstutz12/01/2023Actions

Related issues

Related to Arvados - Idea #20846: Support Ubuntu 22.04 LTSResolvedBrett Smith10/30/2023Actions
Actions #1

Updated by Brett Smith 9 months ago

  • Related to Idea #20846: Support Ubuntu 22.04 LTS added
Actions #2

Updated by Peter Amstutz 9 months ago

  • Target version changed from Future to Development 2024-01-03 sprint
Actions #3

Updated by Brett Smith 8 months ago

The good news is we built our own class that Has-A client underneath, rather than subclassing. Thanks to that, here is the entire API we need to support.

  • __init__ should start a daemon thread to run the connection.
  • Each f argument is a filter.
class EventClient(object):
    def __init__(self, url, filters, on_event_cb, last_log_id): ...
    def subscribe(self, f, last_log_id=None): ...
    def unsubscribe(self, f): ...
    def close(self, code=1000, reason='', timeout=0): ...
    def on_event(self, m): ...
    def on_closed(self): ...
    def run_forever(self): ...
Actions #4

Updated by Brett Smith 8 months ago

We can use the websockets package for this. It has a thread-based client that's pretty compatible with our needs. The main difference is that the API is not callback-oriented, so we'll need to write that glue code ourselves, but that's not hard.

The license is 3-clause BSD so it's compatible with our SDK. Support is available through Tidelift so it's a mature project. Last release was a month ago so it's actively maintained. And of course it's the top result when you search "websockets" on PyPI.

Actions #5

Updated by Brett Smith 8 months ago

  • Assigned To set to Brett Smith
  • Status changed from New to In Progress

I have a branch that passes existing tests. I need to add a couple tests (due to different behavior in the new library) and docs.

Actions #6

Updated by Brett Smith 8 months ago

21146-pysdk-new-websockets @ 187f790d8b71c5c7b54a599bec6e58950d7116dc - developer-run-tests: #3938

remainder rerun developer-run-tests-remainder: #4142 - This has a different failure, so the intersection of both runs pass all tests 🙄

  • All agreed upon points are implemented / addressed.
    • Yes
  • Anything not implemented (discovered or discussed during work) has a follow-up story.
    • N/A
  • Code is tested and passing, both automated and manual, what manual testing was done is described
    • See above
  • Documentation has been updated.
    • Yes
  • Behaves appropriately at the intended scale (describe intended scale).
    • I have not benchmarked to compare performance of ws4py vs. websockets or whatever, but it seems unlikely to be a significant difference.
  • Considered backwards and forwards compatibility issues between client and server.
    • The new EventClient maintains its entire public API, including methods, argument names, attributes, and semantics. Some of the code organization came about specifically to maintain semantics. For example, I thought about moving the last_log_id updating from on_event to run, but then weird things might've happened if someone subclassed EventClient with their own on_event.
  • Follows our coding standards and GUI style guidelines.
    • Yes
Actions #7

Updated by Peter Amstutz 7 months ago

Brett Smith wrote in #note-6:

21146-pysdk-new-websockets @ 187f790d8b71c5c7b54a599bec6e58950d7116dc - developer-run-tests: #3938

remainder rerun developer-run-tests-remainder: #4142 - This has a different failure, so the intersection of both runs pass all tests 🙄

  • All agreed upon points are implemented / addressed.
    • Yes
  • Anything not implemented (discovered or discussed during work) has a follow-up story.
    • N/A
  • Code is tested and passing, both automated and manual, what manual testing was done is described
    • See above
  • Documentation has been updated.
    • Yes
  • Behaves appropriately at the intended scale (describe intended scale).
    • I have not benchmarked to compare performance of ws4py vs. websockets or whatever, but it seems unlikely to be a significant difference.
  • Considered backwards and forwards compatibility issues between client and server.
    • The new EventClient maintains its entire public API, including methods, argument names, attributes, and semantics. Some of the code organization came about specifically to maintain semantics. For example, I thought about moving the last_log_id updating from on_event to run, but then weird things might've happened if someone subclassed EventClient with their own on_event.
  • Follows our coding standards and GUI style guidelines.
    • Yes

This LGTM.

I did a little bit of manual testing with arv-ws and it seems to be working fine.

Actions #8

Updated by Brett Smith 7 months ago

  • Status changed from In Progress to Resolved
Actions #9

Updated by Peter Amstutz 6 months ago

  • Release set to 69
Actions #10

Updated by Brett Smith 6 months ago

Re putting this in 2.7.2: This branch wasn't written with Python 3.7 compatibility in mind. It hopefully wouldn't be a big deal to add, but it would need to be tested and maybe tweaked.

Actions #11

Updated by Peter Amstutz 6 months ago

  • Release deleted (69)
Actions #12

Updated by Peter Amstutz 6 months ago

Per our previous discussion, this was yanked from 2.7.2 and will be in 3.0 instead.

Actions #13

Updated by Peter Amstutz 3 months ago

  • Release set to 70
Actions

Also available in: Atom PDF