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

Add periodic session refresh for Live Video to Video #3404

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Feb 20, 2025

To reduce the startup, we can periodically refresh sessions, then it won't happen when the stream comes in. This could save us ~1-2s startup time.

@leszko leszko requested a review from victorges February 20, 2025 14:29
@github-actions github-actions bot added go Pull requests that update Go code AI Issues and PR related to the AI-video branch. labels Feb 20, 2025
@leszko leszko requested review from mjh1 and emranemran February 20, 2025 14:29
Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Project coverage is 32.13158%. Comparing base (0f9be6a) to head (37485a4).

Files with missing lines Patch % Lines
server/ai_session.go 0.00000% 28 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #3404         +/-   ##
===================================================
- Coverage   32.15038%   32.13158%   -0.01880%     
===================================================
  Files            147         147                 
  Lines          40830       40857         +27     
===================================================
+ Hits           13127       13128          +1     
- Misses         26928       26954         +26     
  Partials         775         775                 
Files with missing lines Coverage Δ
server/ai_session.go 2.07715% <0.00000%> (-0.18091%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f9be6a...37485a4. Read the comment docs.

Files with missing lines Coverage Δ
server/ai_session.go 2.07715% <0.00000%> (-0.18091%) ⬇️

... and 1 file with indirect coverage changes

if err := sel.Refresh(context.Background()); err != nil {
clog.Infof(context.Background(), "Error refreshing AISessionSelector err=%v", err)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we try to be a good citizen and tear down the loop when the node shuts down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, would be good. But I believe it's not so trivial....heheh. I guess we would need to move this to starter.go, etc. Unless you have some simple idea how to exit this loop when the node shuts down?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, or some chan / signal from LivepeerNode ... not a big deal though if it seems more trouble than it is worth.

@ad-astra-video
Copy link
Collaborator

Another issue will likely be that ticket params expire in 10 blocks (about 2 minutes). Refreshing the session will add a couple hundred milliseconds on to startup of request.

I don't think would overly work Orchestrators to run this every 110 seconds. Also don't think its unreasonable to increase the ticket params expiration to be 10 minutes. I think 10 blocks was from L1 days with larger variations in gas fees.

if err := refreshSessionIfNeeded(ctx, sess.BroadcastSession); err != nil {

func refreshSessionIfNeeded(ctx context.Context, sess *BroadcastSession) error {

Also think would be nice to save the LatencyScores at the start of the refresh and add back on to the new sessions. The other PR for the new selector used InitialLatencyScore, where does that come from?

@leszko
Copy link
Contributor Author

leszko commented Feb 21, 2025

Very good points @ad-astra-video !

Another issue will likely be that ticket params expire in 10 blocks (about 2 minutes). Refreshing the session will add a couple hundred milliseconds on to startup of request.
I don't think would overly work Orchestrators to run this every 110 seconds. Also don't think its unreasonable to increase the ticket params expiration to be 10 minutes. I think 10 blocks was from L1 days with larger variations in gas fees.

Hmm, let's maybe do something in between:

Also think would be nice to save the LatencyScores at the start of the refresh and add back on to the new sessions. The other PR for the new selector used InitialLatencyScore, where does that come from?

IIUC the LatencyScores are not changed, because they are used only in knownSessions and we don't change knownSessions during the refresh. About the initial latency, I think it does not matter which we'll use, if the existing one or the new one, they should be similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AI Issues and PR related to the AI-video branch. go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants