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.

Sign up for GitHub

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

Jump to bottom

Veilid Chunking #8558

Merged
merged 36 commits into from Mar 20, 2024
Merged

Veilid Chunking #8558

merged 36 commits into from Mar 20, 2024

Conversation

rasswanth-s
Copy link
Collaborator

@rasswanth-s rasswanth-s commented Mar 7, 2024 •
edited by itstauq

Re-opening @itstauq veilid chunking PR.

Description

Veilid can send at most 32 kb of data in one network call. However the requests-responses from PySyft can be larger than than. This PR contains utilities to send and receive larger data by chunking the data into packets of max 32 kb.

Items to cover in later cuts:

  • Timeout the buffers to prevent memory leaks.
  • Write unit and integration tests
  • Reliability improvements and error recovery (resending failed chunks or handling network interruptions gracefully).
  • What happens in case the servers are behind a load balancer? The VeilidStreamer request can land in any instance but the buffer will not be shared among the instances as everything is in-memory.
    • Does this even needs to be handled?
    • Idea 1: Use sticky connections in the load balancer.
    • Idea 2: Use a pull-based approach for sending the chunks. Sender sends start stream request -> Receiver makes subsequent requests to get all the chunks from the sender.

This PR closes https://github.com/OpenMined/Heartbeat/issues/1076

Affected Dependencies

None

How has this been tested?

  • Using tests present in notebooks/Testing/Veilid/Large-Message-Testing.ipynb

Checklist

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@itstauq
add architecture diagram for VeilidStreamer
219ee62
@itstauq
Integrate VeilidStreamer with veilid_core
78c1d95 
Make VeilidStreamer a singleton
Add notebooks to test sending large messages using /app_call endpoint
@itstauq itstauq self-assigned this Mar 8, 2024
@rasswanth-s rasswanth-s changed the title Veilid Chunking [WIP] Veilid Chunking Mar 11, 2024
Base automatically changed from rasswanth/veilid-prototype to dev March 13, 2024 08:33
@itstauq itstauq changed the title [WIP] Veilid Chunking Veilid Chunking Mar 19, 2024
@itstauq
Add suggestions from PR comments
105e1c3 
- Change Call ID to Stream ID throughout VeilidStreamer
- Clean up VeilidStreamer.__init__ method by moving the initialization of structs, message sizes, and semaphores to separate methods
- Clean up the VeilidStreamer.stream method by moving the logic to wait for reply and sending single and multi chunk requests to separate methods
- Add a new method to cleanup the buffer after the stream is complete
- Other minor changes
@itstauq
Ensure that all the chunks are received before reassembling the message
0d07473
@itstauq
Merge branch 'dev' into tauquir/veilid-chunking
ab741db
@rasswanth-s
Copy link
Collaborator Author

Stellar Work @itstauq ⭐
It's great to Chunking working with great reliability
test

@rasswanth-s rasswanth-s merged commit b04db1f into dev Mar 20, 2024
34 checks passed
@rasswanth-s rasswanth-s deleted the tauquir/veilid-chunking branch March 20, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itstauq itstauq itstauq approved these changes

Assignees

@itstauq itstauq

Labels
0.8.6 Purple Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rasswanth-s @itstauq @madhavajay