Skip to content

refactor: cleanup server/runner - #287

Merged
jtechapps merged 1 commit into
llm-d:mainfrom
evacchi:refactor-cleanup-runner
Jul 6, 2026
Merged

refactor: cleanup server/runner#287
jtechapps merged 1 commit into
llm-d:mainfrom
evacchi:refactor-cleanup-runner

Conversation

@evacchi

@evacchi evacchi commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Refactor Runner.Run() so that the logic is more straightforward: move some code to sibling funcs, refactor to a unified defer block, share the same context where possible.

Why is this change needed?

Runner.Run() is hard to understand and would benefit from a light refactoring, before we move more pieces around.

How was this tested?

All tests should still pass.

  • Unit tests added/updated
  • Integration/e2e tests added/updated
  • Manual testing performed

Checklist

  • Commits are signed off (git commit -s) per DCO
  • Code follows project contributing guidelines
  • Tests pass locally (make test)
  • Linters pass (make lint)
  • Documentation updated (if applicable)

Related Issues

follow up to #283, related to #284

@evacchi
evacchi requested a review from shimib as a code owner June 25, 2026 15:52
Copilot AI review requested due to automatic review settings June 25, 2026 15:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors Runner.Run() to make the startup/shutdown flow easier to follow by extracting cohesive helper functions and consolidating cleanup into a single deferred shutdown block.

Changes:

  • Refactored Runner.Run() into a more linear sequence with helper funcs for tracer init, pool loading, flow construction, health server init, metrics server start, inference client init, and transform loading.
  • Centralized shutdown/cleanup (health server, gate factory, tracer) into one deferred block that consistently runs on all exit paths.
  • Improved a few error returns (notably wrapping TLS build errors and transform config/chain errors with %w).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@shimib

shimib commented Jun 25, 2026

Copy link
Copy Markdown
Member

The refactor dropped the setupLog.Error(...) logs from loadWorkerPools, loadFlow, initInferenceClient (TLS), and
loadTransforms — they now just return err, and main.go handles a Run() error with a bare os.Exit(1). So a bad pool/flow/TLS/transform config now exits with no output.
Since Run has a named return, maybe log once in the existing defer:

defer func() {
if err != nil {
setupLog.Error(err, "runner failed")
}
// ...existing shutdown...
}()

Nits (non-blocking): loadTransforms should take ctx first; naked return → return nil; the new pure helpers (loadFlow, loadWorkerPools) are worth a quick unit test. The pool not found removal is fine — the merge policy already panics on an unknown pool.

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi
evacchi force-pushed the refactor-cleanup-runner branch from eb62c01 to b99a773 Compare June 28, 2026 16:57
@evacchi evacchi mentioned this pull request Jun 29, 2026
8 tasks
@jtechapps
jtechapps merged commit 2a52e32 into llm-d:main Jul 6, 2026
9 of 10 checks passed
@evacchi
evacchi deleted the refactor-cleanup-runner branch July 30, 2026 17:14
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.

4 participants