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

Update ffi to reflect proto2 syntax changes #290

Merged
merged 6 commits into from
Oct 21, 2024
Merged

Update ffi to reflect proto2 syntax changes #290

merged 6 commits into from
Oct 21, 2024

Conversation

lukasIO
Copy link
Contributor

@lukasIO lukasIO commented Oct 17, 2024

No description provided.

@lukasIO lukasIO requested a review from theomonnom October 17, 2024 10:08
@lukasIO lukasIO marked this pull request as ready for review October 21, 2024 08:28
@lukasIO lukasIO requested a review from bcherry October 21, 2024 08:28
@lukasIO
Copy link
Contributor Author

lukasIO commented Oct 21, 2024

@theomonnom any idea why the eventemitter test might be failing? This PR doesn't include any changes around eventemitter 🤔

Comment on lines +295 to 304
existing_attributes = {
entry.key: entry.value for entry in req.set_local_attributes.attributes
}
existing_attributes.update(attributes)

for key, value in existing_attributes.items():
entry = req.set_local_attributes.attributes.add()
entry.key = key
entry.value = value

Copy link
Member

Choose a reason for hiding this comment

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

Was the logic wrong before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it showed up as a type error where lists and dicts were mixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but this might also be as a result of how the map type in proto get's interpreted in proto2 vs proto3 syntax, not sure

@theomonnom
Copy link
Member

@theomonnom any idea why the eventemitter test might be failing? This PR doesn't include any changes around eventemitter 🤔

That's so weird, it is passing on main, will take a look

@lukasIO lukasIO merged commit 8d402e0 into main Oct 21, 2024
10 of 11 checks passed
@lukasIO lukasIO deleted the lukas/ffi-update branch October 21, 2024 19:25
@bcherry
Copy link
Contributor

bcherry commented Oct 23, 2024

@lukasIO @theomonnom I see a similar issue here as in Node - a runtime error about missing required fields when running the basic_room example:

(venv) @13:15:32 ~/dev/livekit/python-sdks/examples [main]: test-creds-local python basic_room.py
/Users/ben/dev/livekit/python-sdks/examples/basic_room.py:154: DeprecationWarning: There is no current event loop
  loop = asyncio.get_event_loop()
ERROR:asyncio:Task exception was never retrieved
future: <Task finished name='Task-1' coro=<main() done, defined at /Users/ben/dev/livekit/python-sdks/examples/basic_room.py:12> exception=EncodeError('Message livekit.proto.FfiRequest is missing required fields: connect.options.adaptive_stream,connect.options.join_retries')>
Traceback (most recent call last):
  File "/Users/ben/dev/livekit/python-sdks/examples/basic_room.py", line 140, in main
    await room.connect(os.getenv("LIVEKIT_URL"), token)
  File "/Users/ben/dev/livekit/python-sdks/livekit-rtc/livekit/rtc/room.py", line 364, in connect
    resp = FfiClient.instance.request(req)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ben/dev/livekit/python-sdks/livekit-rtc/livekit/rtc/_ffi_client.py", line 219, in request
    proto_data = req.SerializeToString()
                 ^^^^^^^^^^^^^^^^^^^^^^^
google.protobuf.message.EncodeError: Message livekit.proto.FfiRequest is missing required fields: connect.options.adaptive_stream,connect.options.join_retries
INFO:livekit:livekit_ffi::server:137:livekit_ffi::server - initializing ffi server v0.11.3
INFO:livekit:livekit_ffi::cabi:27:livekit_ffi::cabi - initializing ffi server v0.11.3

@lukasIO
Copy link
Contributor Author

lukasIO commented Oct 24, 2024

When implementing the proto2 syntax I made all fields required that weren't explicitly marked as optional before. Maybe this also overdid things for the RoomOptions? Not sure if these missing fields should really be required for the FFI request, wdyt @theomonnom ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants