Skip to content

Conversation

@Olshansk
Copy link
Collaborator

@Olshansk Olshansk commented Sep 30, 2025

Reviewing Streaming Support for SSE and NDJSON (used for LLMs) in #1750.

Next steps:

Reproducing this locally (after running localnet):

make send_relay_path_REST
make pocketd_relayminer_relay_JSONRP

Changes made:

  • Updated the LocalNet toolchain to make it easier to test REST & LLM services
  • Added make pocketd_relayminer_relay_JSONRPC and revived make send_relay_path_REST
  • Reviewing streaming RelayMiner changes in #1570 for code health & maintainbility
  • Moved logic from sync.go into server.handleHttp to follow a similar pattern as server.handleHttpStream

The image below shows the result in LocalNet:

Screenshot 2025-09-30 at 8 10 22 PM

@Olshansk Olshansk self-assigned this Sep 30, 2025
@Olshansk Olshansk added the review A PR that's intended as a review of another PR label Sep 30, 2025
@Olshansk Olshansk added this to Shannon Sep 30, 2025
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Shannon Sep 30, 2025
@Olshansk Olshansk moved this from 📋 Backlog to 👀 In review in Shannon Sep 30, 2025
@Olshansk Olshansk added this to the RelayMiner Stability milestone Sep 30, 2025
@Olshansk Olshansk changed the base branch from main to streaming-support September 30, 2025 21:25
@Olshansk Olshansk changed the title [Review] Reviewiewing #1570 [Review] Reviewing #1570: Support for Support for SSE and NDJSON Oct 1, 2025
@Olshansk Olshansk requested a review from red-0ne October 1, 2025 03:29
@Olshansk Olshansk requested a review from RawthiL October 1, 2025 03:30
@Olshansk Olshansk added relayminer Changes related to the Relayminer code health Cleans up some code labels Oct 1, 2025
@Olshansk Olshansk marked this pull request as ready for review October 1, 2025 03:30
@RawthiL
Copy link
Contributor

RawthiL commented Oct 13, 2025

This PR seems to contain a total re-work of the code, should I review this new implementation instead of using the one I did?

@RawthiL RawthiL force-pushed the streaming-support branch 2 times, most recently from f61ad03 to e98915f Compare October 13, 2025 19:52
@RawthiL
Copy link
Contributor

RawthiL commented Oct 13, 2025

I updated the base to latest main, so now this needs to be rebased so I can evaluate the logic changes (if any). I'm not sure if the streaming support was actually tested here. The images provided do not show relays performing streaming requests.

I think the approach of creating handlers per request (server.handleHttp/server.handleHttpStream) is correct. I have not implemented it so far because it makes rebases a hell..

I see lots of config changes but basically it seems that you are adding just two rules:

  • send_relay_path_REST : Send a relay with path to Ollama (I cannot test if any of this works)
  • pocketd_relayminer_relay_JSONRPC : Which sends a blockchain RPC to relayminer.
    I could add a third rule to make an RPC using pocketd relayminer relay to an overridden endpoint that can point to any OpenAI API compatibel endpoint. This way we will be able to test it without needing to deploy PATH or Ollama. Will that be enough?

P.S.: I see the header timeout removed, great..

@Olshansk Olshansk merged commit c7b0905 into streaming-support Oct 16, 2025
9 of 10 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Shannon Oct 16, 2025
@Olshansk Olshansk deleted the streaming-support-review branch October 16, 2025 18:49
RawthiL added a commit that referenced this pull request Oct 16, 2025
…1805)

Reviewing `Streaming Support for SSE and NDJSON (used for LLMs)` in #1750. 

**Next steps**:
- Outlined in this comment: #1570 (review)

**Reproducing this locally (after running localnet)**:

```bash
make send_relay_path_REST
make pocketd_relayminer_relay_JSONRP
```


**Changes made**:
- Updated the LocalNet toolchain to make it easier to test REST & LLM services
- Added `make pocketd_relayminer_relay_JSONRPC` and revived `make send_relay_path_REST`
- Reviewing streaming RelayMiner changes in [#1570](#1570) for code health & maintainbility
- Moved logic from `sync.go` into `server.handleHttp` to follow a similar pattern as `server.handleHttpStream`

---

The image below shows the result in LocalNet:

<img width="3552" height="1527" alt="Screenshot 2025-09-30 at 8 10 22 PM" src="https://github.com/user-attachments/assets/bcf9b2dc-5560-4ab4-b06f-c1ebdfaaaa93" />

---

Co-authored-by: Ramiro Rodriguez Colmeiro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code health Cleans up some code relayminer Changes related to the Relayminer review A PR that's intended as a review of another PR

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants