Add awareness features to handle server state #170

brichet · 2024-10-03T10:04:59Z

~~This PR adds the awareness class, extracted from https://github.com/jupyter-server/pycrdt-websocket.~~

This PR adds some features to that awareness to allow the server to update and observe it.

davidbrochart

Thanks @brichet!
Now that pycrdt has an API reference documentation, all public classes and methods should have docstrings.
Also, we'll need 100% test coverage. You can check with:

coverage run -m pytest tests
coverage report --show-missing --fail-under=100

tests/test_awareness.py

pyproject.toml

python/pycrdt/_awareness.py

brichet · 2024-10-03T15:08:38Z

Also, we'll need 100% test coverage.

It should be OK now

python/pycrdt/_awareness.py

…ome features to it

for more information, see https://pre-commit.ci

Co-authored-by: David Brochart <[email protected]>

python/pycrdt/_awareness.py

Co-authored-by: David Brochart <[email protected]>

python/pycrdt/_awareness.py

davidbrochart · 2024-10-04T14:55:34Z

@brichet I took the liberty to make some changes in 98540a5, please let me know if you agree with them.
Among other things, I removed the on_change() callback. The encoded state is just (optionally) returned when the local state is set. That lets the door open for emitting the encoded state in the future.

brichet · 2024-10-07T09:30:36Z

Thanks for the suggestion, I can take a look at it..
Maybe we should keep the get_change() as deprecated to avoid backward incompatibility.

…he changes

brichet · 2024-10-07T14:23:55Z

@davidbrochart I updated the class according to your suggestions, that should be closer to awareness.js now.

davidbrochart · 2024-10-07T14:28:17Z

Thanks, do you mind if I take it from now? It will be quicker than requesting changes, and you will still be able to accept/reject my changes afterwards.

davidbrochart · 2024-10-08T08:10:05Z

@brichet Let me know what you think about c75c8f9.

brichet

Thanks @davidbrochart.

Does it work on your side ? I have errors when I try to use it:

      File "/home/brichet/projects/pycrdt/python/pycrdt/_awareness.py", line 136, in apply_awareness_update
        state_str = decoder.read_var_string()
                    ^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/brichet/projects/pycrdt/python/pycrdt/_sync.py", line 227, in read_var_string
        return message.decode("utf-8")
               ^^^^^^^^^^^^^^^^^^^^^^^
    UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc0 in position 0: invalid start byte

brichet · 2024-10-08T09:21:34Z

python/pycrdt/_awareness.py

-    def _update_states(
-        self, client_id: int, clock: int, state: Any, states_changes: dict[str, list[int]]
-    ) -> None:


I think this function could be kept, using an origin to know if we should remove the local state or not.

But it's slightly different between setLocalState and applyAwarenessUpdate, right? In particular setLocalState doesn't handle the clock.

Not sure to understand "handle the clock". It does update it too, no ?

But what about this, that _update_states() was doing inconditionally?

I agree that it was wrong in _update_states(), it should also have tested that origin is not "local".
Anyway, the current implementation works, it was just to avoid duplicating code.

davidbrochart · 2024-10-08T09:34:30Z

Does it work on your side ? I have errors when I try to use it:

      File "/home/brichet/projects/pycrdt/python/pycrdt/_awareness.py", line 136, in apply_awareness_update
        state_str = decoder.read_var_string()
                    ^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/brichet/projects/pycrdt/python/pycrdt/_sync.py", line 227, in read_var_string
        return message.decode("utf-8")
               ^^^^^^^^^^^^^^^^^^^^^^^
    UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc0 in position 0: invalid start byte

Do you have a reproducible example?

brichet · 2024-10-08T09:38:45Z

Do you have a reproducible example?

Not really, I have an environment with dev install of pycrdt, pycrdt_websocket, jupyter_collaboration and jupyter_ydoc. All these packages need to be updated to test it properly.
I'll try to figure out where the error come from.

brichet · 2024-10-08T10:27:23Z

python/pycrdt/_awareness.py

+        if state is None:
+            del self._states[client_id]


This can probably raise an error if the local state has not been defined and we try to set it as None.

Raising an exception is a feature IMO (rather than keep it silent), but maybe you mean that we should raise a more meaningful exception?

I don't think we need to raise an exception, if we want to set it to null but it is already the case, it might be silent, like here

But in set_local_state we are responsible for what we do, in apply_awareness_update less so. But I don't have a strong opinion on this, I will add the check back 👍

brichet · 2024-10-08T10:36:32Z

python/pycrdt/_awareness.py

+                        # Remote client removed the local state. Do not remove state.
+                        # Broadcast a message indicating that this client still exists by increasing
+                        # the clock.
+                        clock += 1


Shouldn't it be

clock = curr_clock + 1

to update the local clock ?

This mimics the JavaScript implementation. Should it be different?

Maybe I misunderstood the clock, but I understood that each client has its own.
If this is the case, why should we rely on a value coming from a client to update the local clock ?

Do you want to open an issue in https://github.com/yjs/y-protocols to discuss this, or maybe @dmonad can comment here?

brichet · 2024-10-08T10:38:01Z

python/pycrdt/_awareness.py

+                elif client_meta is not None and state is None:
+                    removed.append(client_id)


Will we not fallback here if a remote client try to remove the local state ?

It's the translation of this. Do you mean that there is an issue in the JavaScript implementation?

Maybe...
We should test it, but it seems to me that if a remote client try to set the local awareness to null, this condition is fulfilled.
The client_meta is the local meta (and may be not null), and the state is the state to apply, which is null in that case.

But a remote client cannot remove the local state, right?

I agree with that, but it will send a wrong information to other client, that the local state has been removed, no ?

I don't know, maybe open a PR in https://github.com/yjs/y-protocols? It should be fixed there too, and they will confirm if it's a bug.

brichet · 2024-10-08T14:26:17Z

Thanks @davidbrochart.

Does it work on your side ? I have errors when I try to use it:

      File "/home/brichet/projects/pycrdt/python/pycrdt/_awareness.py", line 136, in apply_awareness_update
        state_str = decoder.read_var_string()
                    ^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/brichet/projects/pycrdt/python/pycrdt/_sync.py", line 227, in read_var_string
        return message.decode("utf-8")
               ^^^^^^^^^^^^^^^^^^^^^^^
    UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc0 in position 0: invalid start byte

@davidbrochart it seems that we do not take into account the full length of the message (first integer), when encoding and decoding.
This call was removing the first integer, corresponding to the length in bytes of the message, but has been remove in a previous commit.
And when encoding, we should add the full length before returning the message (https://github.com/jupyter-server/pycrdt/pull/170/files#diff-d0908cdc750b8113ae6b4433e378db136aebe96a30e773aef2cf08313b6669d8R117), I observe the same error in console.

davidbrochart · 2024-10-08T14:46:02Z

Good catch!
The thing is I don't know if if should be part of encode_awareness_update and apply_awareness_update. It doesn't seem to be the case in awareness.js. Since this PR is already a breaking change, maybe we should let it as-is, and do the job outside?

davidbrochart · 2024-10-08T14:51:09Z

An alternative would be to pass from_message/to_message boolean parameters, that would do the extra work if True.

brichet · 2024-10-08T15:04:19Z

An alternative would be to pass from_message/to_message boolean parameters, that would do the extra work if True.

I can't see a use case where we need an 'incomplete' encoded awareness (without the length).
Maybe we don't have to mimic exactly what is done in awareness.js if it is not relevant.

Also the YMessageType is missing from the encoder, and again I don't really know when encode_awareness_update() can be called for another message type....

davidbrochart · 2024-10-09T07:21:37Z

An update and a message are two different things, the latter encapsulates the former. For instance, Doc.get_update() is a document update that also has to be encapsulated in a message, using create_message().
So I think encode_awareness_update() and apply_awareness_update() are fine like that, they do what their name implies. But I can add a create_awareness_message(update) function (read_message() can be used to get an awareness update from a message).

davidbrochart

I think it's good to go, any objection to merge @brichet?

brichet · 2024-10-09T08:43:01Z

I think it's good to go, any objection to merge @brichet?

Yes, it looks good to me too.

davidbrochart · 2024-10-09T09:23:00Z

Thanks @brichet.

brichet · 2024-10-09T09:26:37Z

Thanks for helping a lot on it @davidbrochart.

davidbrochart · 2024-10-09T09:38:24Z

Released in v0.10.0.

brichet mentioned this pull request Oct 3, 2024

Broadcast server awareness to all clients jupyter-server/pycrdt-websocket#73

Merged

davidbrochart requested changes Oct 3, 2024

View reviewed changes

tests/test_awareness.py Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

python/pycrdt/_awareness.py Outdated Show resolved Hide resolved

davidbrochart requested changes Oct 3, 2024

View reviewed changes

python/pycrdt/_awareness.py Outdated Show resolved Hide resolved

davidbrochart requested changes Oct 3, 2024

View reviewed changes

brichet mentioned this pull request Oct 3, 2024

Doc awareness jupyter-server/jupyter_ydoc#277

Merged

brichet and others added 10 commits October 4, 2024 10:14

Move the Awareness from pycrdt_websocket to pycrdt project, and add s…

36c0dfb

…ome features to it

Add tests on awareness

0df6b20

use google style docstring

ac39dae

Generate the message in test for clarity

8c3d21b

Add docstring and tests

a183971

[pre-commit.ci] auto fixes from pre-commit.com hooks

888dca9

for more information, see https://pre-commit.ci

remove the unused logger

879b9ad

Remove typing from test

bf148d2

Apply suggestions from code review

6d02368

Co-authored-by: David Brochart <[email protected]>

Add missing docstring

14ac84f

brichet force-pushed the awareness branch from d20e98b to 14ac84f Compare October 4, 2024 08:18

brichet changed the title ~~Add awareness~~ Add awareness features to handle server state Oct 4, 2024

davidbrochart reviewed Oct 4, 2024

View reviewed changes

python/pycrdt/_awareness.py Outdated Show resolved Hide resolved

davidbrochart reviewed Oct 4, 2024

View reviewed changes

python/pycrdt/_awareness.py Outdated Show resolved Hide resolved

davidbrochart reviewed Oct 4, 2024

View reviewed changes

python/pycrdt/_awareness.py Outdated Show resolved Hide resolved

brichet and others added 3 commits October 4, 2024 11:03

Apply suggestions from code review

4fc8c93

Co-authored-by: David Brochart <[email protected]>

Remove the default user in the awareness

5bde016

Remove totally the conept of user in the awareness

f523e3d

davidbrochart requested changes Oct 4, 2024

View reviewed changes

brichet and others added 3 commits October 4, 2024 12:03

Add subscription id

6b06af1

update docstring according to review

edb66a5

Remove on_change callback

98540a5

davidbrochart force-pushed the awareness branch from 1b74f22 to 98540a5 Compare October 4, 2024 14:55

brichet added 2 commits October 7, 2024 16:00

Observe both local and remote changes, and add a function to encode t…

3d4beaf

…he changes

mypy

0a37d17

davidbrochart force-pushed the awareness branch from e531376 to 982970a Compare October 8, 2024 08:08

Mimic awareness.js

c75c8f9

davidbrochart force-pushed the awareness branch from 4ad3ccb to c75c8f9 Compare October 8, 2024 08:09

brichet commented Oct 8, 2024

View reviewed changes

Check if state is set before deleting

dc4468d

Add create_awareness_message() and write_message()

b02eb3b

davidbrochart force-pushed the awareness branch from 4f721ab to b02eb3b Compare October 9, 2024 07:53

davidbrochart approved these changes Oct 9, 2024

View reviewed changes

davidbrochart merged commit fd74268 into jupyter-server:main Oct 9, 2024
27 checks passed

brichet deleted the awareness branch October 9, 2024 09:26

		elif client_meta is not None and state is None:
		removed.append(client_id)

Add awareness features to handle server state #170

Add awareness features to handle server state #170

Conversation

brichet commented Oct 3, 2024 • edited Loading

davidbrochart left a comment

Choose a reason for hiding this comment

brichet commented Oct 3, 2024

davidbrochart commented Oct 4, 2024

brichet commented Oct 7, 2024

brichet commented Oct 7, 2024

davidbrochart commented Oct 7, 2024

davidbrochart commented Oct 8, 2024

brichet left a comment • edited Loading

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

davidbrochart commented Oct 8, 2024

brichet commented Oct 8, 2024 • edited Loading

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

Choose a reason for hiding this comment

brichet commented Oct 8, 2024 • edited Loading

davidbrochart commented Oct 8, 2024

davidbrochart commented Oct 8, 2024

brichet commented Oct 8, 2024

davidbrochart commented Oct 9, 2024

davidbrochart left a comment

Choose a reason for hiding this comment

brichet commented Oct 9, 2024

davidbrochart commented Oct 9, 2024

brichet commented Oct 9, 2024

davidbrochart commented Oct 9, 2024

brichet commented Oct 3, 2024 •

edited

Loading

brichet left a comment •

edited

Loading

brichet commented Oct 8, 2024 •

edited

Loading

brichet commented Oct 8, 2024 •

edited

Loading