Replace custom models with msgspec #2157

jodal · 2024-03-10T00:04:20Z

I would very much like to get rid of our own custom model implementation.

This is a proof of concept using the msgspec library to implement our models.
It passes all tests and linters, and it is still able to play music.

I'm not married to msgspec yet, but I think this was still worth it, as
switching to e.g. Pydantic would probably mostly require us to rewrite the
models themselves, and not all the other fixes I did.

Requirements

Memory usage

One of the primary goals of our old bespoke model implementation was to save
memory.

By switching to msgspec, our models still take the same amount of memory as
slot classes. However, we no longer cache identical models.

I have not measured the difference this makes.

Validation at deserialization

We need to validate input data, e.g. from HTTP clients. This is now done by
msgspec, whose JSON decoder is now used for all HTTP requests.

Validation at object instantiation

msgspec does not do any validation on object instantiation. This is mostly
something we do in our tests, and there are still a lot of passing tests doing
things that the type checker is not happy about.

Since the type checker points out where the problems are, I am not certain we
need runtime validation of Python object instantiation.

If we want to be really strict and crash early, where the objects are
instantiated (e.g. in extensions) and not where they are used (e.g. core), then
we should probably switch to something like Pydantic, that does validation at
object instantiation.

Debian packaging

Neither msgspec or Pydantic v2 is packed in Debian.

Pydantic v1: We could go with Pydantic v1 for now, until v2 is packaged. That
would be a tiny upgrade later in time.

Road ahead

Please test this, and help find issues.
Let's continue discussing exactly which library we want to use for our models. They all have pros and cons. This is not a clear choice.

To avoid import cycles, we need this basic type to be available without importing the full audio system.

We need to do a major revamp of the model creation in our test suite before we make URI required.

jodal · 2024-03-10T00:32:27Z

I see that I've broken Mopidy-Iris by removing the JSON encoder/decoder. I think I might need to reintroduce those anyway to unbreak web clients in general, as I don't think I can have msgspec decode something so generic as our core API method params, with lists and dicts of models.

To be able to go from model name to class.

Thanks to tips in jcrist/msgspec#652.

kingosticks · 2024-03-11T15:25:15Z

The HTTP frontend seems to be sending empty responses:

DEBUG    2024-03-11 15:18:50,969 [10264:HttpServer] mopidy.http.handlers
  Received WebSocket message from 127.0.0.1: '{"method":"core.tracklist.set_consume","params":{"value":true},"jsonrpc":"2.0","id":24}'
DEBUG    2024-03-11 15:18:50,969 [10264:HttpServer] mopidy.http.handlers
  Sent WebSocket message to 127.0.0.1: b'{"jsonrpc":"2.0","id":24}'

Which mopidy.js doesn't like:

Error: Response without 'result' or 'error' received

EDIT:
Looking again now I have more time, the request my client is doing is maybe what's at fault here. Maybe this method should be a JSON-RPC notification since it doesn't return anything. In which case my request shoudn't be sending an id field. Not sure what's going wrong here, it should be using latest mopidy.js for this.

kingosticks · 2024-03-11T22:29:51Z

The HTTP frontend seems to be sending empty responses:
DEBUG    2024-03-11 15:18:50,969 [10264:HttpServer] mopidy.http.handlers
  Received WebSocket message from 127.0.0.1: '{"method":"core.tracklist.set_consume","params":{"value":true},"jsonrpc":"2.0","id":24}'
DEBUG    2024-03-11 15:18:50,969 [10264:HttpServer] mopidy.http.handlers
  Sent WebSocket message to 127.0.0.1: b'{"jsonrpc":"2.0","id":24}'
Which mopidy.js doesn't like:

Error: Response without 'result' or 'error' received

EDIT: Looking again now I have more time, the request my client is doing is maybe what's at fault here. Maybe this method should be a JSON-RPC notification since it doesn't return anything. In which case my request shoudn't be sending an id field. Not sure what's going wrong here, it should be using latest mopidy.js for this.

Nope! That was wrong. All API methods in mopidy.js are requests, not notifications. And the old RPC handler works OK with the same request.

DEBUG    2024-03-11 22:00:54,535 [82011:HttpServer] mopidy.http.handlers
  Received WebSocket message from 127.0.0.1: '{"method":"core.tracklist.set_consume","params":{"value":true},"jsonrpc":"2.0","id":18}'
DEBUG    2024-03-11 22:00:54,535 [82011:HttpServer] mopidy.http.handlers
  Sent WebSocket message to 127.0.0.1: '{"jsonrpc": "2.0", "id": 18, "result": null}'

Seems this new handler omits a None result from the response, presumably because msgspec omit_defaults is used. But that doesn't follow the spec:

result
This member is REQUIRED on success.
This member MUST NOT exist if there was an error invoking the method.

So we probably need the response class to be more like the old version?

jodal · 2024-03-12T20:36:38Z

I added a test case and made sure JSON-RPC responses with None results are serialized, like we discussed in jodal#1.

kingosticks · 2024-03-13T23:54:27Z

As discussed on Zulip, the JSON-RPC parameter decoding doesn't work for dict type parameters. e.g.

For example, for search:

DEBUG    2024-03-13 23:50:37,464 [3220:HttpServer] mopidy.http.handlers
  Received WebSocket message from 127.0.0.1: '{"method":"core.library.search","params":{"query":{"any":["abba"]},"uris":["spotify:"]},"jsonrpc":"2.0","id":16}'
DEBUG    2024-03-13 23:50:37,464 [3220:HttpServer] mopidy.http.handlers
  Sent WebSocket message to 127.0.0.1: b'{"jsonrpc":"2.0","id":null,"error":{"code":-32600,"message":"Invalid Request","data":"Object missing required field `__model__` - at `$.params[...]`"}}'

And this response isn't handled well by mopidyjs because it's missing the id field (but that's a slightly different problem):

Unexpected response received. Message was: 
Object { jsonrpc: "2.0", id: null, error: {…} }
    error: Object { code: -32600, message: "Invalid Request", data: "Object missing required field `__model__` - at `$.params[...]`" }
    id: null
    jsonrpc: "2.0"

kingosticks · 2024-03-15T18:04:33Z

src/mopidy/internal/jsonrpc.py

+                type=Request | list[Request],
+            )
+        except msgspec.ValidationError as exc:
+            response = JsonRpcInvalidRequestError(data=str(exc)).get_response()


Suggested change

response = JsonRpcInvalidRequestError(data=str(exc)).get_response()

# Try and get any request ID that was present (non-batched only).

# This JSON is safe to decode.

id = msgspec.json.decode(request_json).get('id')

response = JsonRpcInvalidRequestError(data=str(exc)).get_response(id)

kingosticks · 2024-03-15T18:07:42Z

src/mopidy/internal/jsonrpc.py

-
-JsonRpcRequestId: TypeAlias = str | int | float
-
+    params: list[Param] | dict[str, Param] | None = None


Suggested change

params: list[Param] | dict[str, Param] | None = None

params: list[Param] | dict[str, Any] | None = None

Workaround for methods that have a dict parameter:

https://docs.mopidy.com/latest/api/core/#mopidy.core.TracklistController.filter

https://docs.mopidy.com/latest/api/core/#mopidy.core.LibraryController.search

https://docs.mopidy.com/latest/api/core/#mopidy.core.LibraryController.get_distinct

kingosticks · 2024-03-15T18:08:29Z

src/mopidy/internal/jsonrpc.py

+    def build(
+        cls,
+        method: str,
+        params: list[Param] | dict[str, Param] | None = None,


Suggested change

params: list[Param] | dict[str, Param] | None = None,

params: list[Param] | dict[str, Any] | None = None,

Goes with https://github.com/mopidy/mopidy/pull/2157/files#r1526655797

jodal added 18 commits March 9, 2024 18:45

Add msgspec as dependency

e24b859

types: Reorder

01f97e7

types: Move PlaybackState to mopidy.types

50596fd

To avoid import cycles, we need this basic type to be available without importing the full audio system.

types: Add DateOrYear type

f17f00a

types: Add validation pattern for Uri and UriSchema

b65836e

types: Add NonNegativeInt

5993458

types: Use Uri type in more places

95f62fa

models: Reimplement with msgspec

2d8a194

models: Fix type of collections passed into models

4805a3f

models: Split tests

c12397e

core: Fix types for state persistence

7baddef

core: Make TlTrack unpacking typesafe

bb4e654

core: Use TracklistId type instead of int

bfad1e3

core: Use msgspec for persisted state

7fce673

core: Update tests

63f5bdf

jsonrpc: Use msgspec for JSON encoding/decoding

27c7828

m3u: Update tests

f9ca597

Return early if model's URI field is unset

53e6e08

We need to do a major revamp of the model creation in our test suite before we make URI required.

jodal requested a review from kingosticks March 10, 2024 00:04

Update changelog

de0b4b7

jodal added 5 commits March 10, 2024 12:12

models: Convert from a module to a package again

c0d47a4

models: Register models in a registry

bbaa084

To be able to go from model name to class.

models: Reintroduce JSON encoder/decoder helpers

9040ab7

models: Remove empty collections from repr/serialization

30ecfa8

Thanks to tips in jcrist/msgspec#652.

jsonrpc: Handle lists as params

e3a4323

jsonrpc: Do not omit None results from serialization

0fde81e

kingosticks reviewed Mar 15, 2024

View reviewed changes

benreid24 mentioned this pull request Mar 20, 2024

Add LibraryController.identify method #2159

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace custom models with msgspec #2157

Replace custom models with msgspec #2157

jodal commented Mar 10, 2024

jodal commented Mar 10, 2024

kingosticks commented Mar 11, 2024 •

edited

kingosticks commented Mar 11, 2024

jodal commented Mar 12, 2024

kingosticks commented Mar 13, 2024 •

edited

kingosticks Mar 15, 2024

kingosticks Mar 15, 2024 •

edited

kingosticks Mar 15, 2024

-            response = JsonRpcInvalidRequestError(data=str(exc)).get_response()
+            # Try and get any request ID that was present (non-batched only).
+            # This JSON is safe to decode.
+            id = msgspec.json.decode(request_json).get('id')
+            response = JsonRpcInvalidRequestError(data=str(exc)).get_response(id)


		JsonRpcRequestId: TypeAlias = str \| int \| float

		params: list[Param] \| dict[str, Param] \| None = None

	params: list[Param] \| dict[str, Param] \| None = None
	params: list[Param] \| dict[str, Any] \| None = None

Replace custom models with msgspec #2157

Are you sure you want to change the base?

Replace custom models with msgspec #2157

Conversation

jodal commented Mar 10, 2024

Requirements

Memory usage

Validation at deserialization

Validation at object instantiation

Debian packaging

Road ahead

jodal commented Mar 10, 2024

kingosticks commented Mar 11, 2024 • edited

kingosticks commented Mar 11, 2024

jodal commented Mar 12, 2024

kingosticks commented Mar 13, 2024 • edited

kingosticks Mar 15, 2024

Choose a reason for hiding this comment

kingosticks Mar 15, 2024 • edited

Choose a reason for hiding this comment

kingosticks Mar 15, 2024

Choose a reason for hiding this comment

kingosticks commented Mar 11, 2024 •

edited

kingosticks commented Mar 13, 2024 •

edited

kingosticks Mar 15, 2024 •

edited