Skip to content

feat: Add support for uvicorn workers for llama-stack #211

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
Open
VaishnaviHire wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Add support for uvicorn workers for llama-stack #211

VaishnaviHire wants to merge 1 commit into from

Conversation

@VaishnaviHire
Copy link
Collaborator

@VaishnaviHire VaishnaviHire commented Dec 5, 2025 •
edited
Loading

Fixes #164
Expose worker field in LlamaStackDistribution CR

Example CR

spec:
  server:
    containerSpec:
      env:
        - name: OLLAMA_INFERENCE_MODEL
          value: 'llama3.2:1b'
        - name: OLLAMA_URL
          value: 'http://ollama-server-service.ollama-dist.svc.cluster.local:11434'
      name: llama-stack
    distribution:
      name: starter
    workers: 2

@VaishnaviHire VaishnaviHire force-pushed the add_workers branch 2 times, most recently from 5c03fe0 to af4d5f7 Compare December 5, 2025 21:04
@VaishnaviHire
Copy link
Collaborator Author

VaishnaviHire commented Dec 8, 2025

@mergify rebase

@mergify
Copy link

mergify bot commented Dec 8, 2025

rebase

✅ Branch has been successfully rebased

leseb
leseb requested changes Dec 8, 2025
View reviewed changes
Copy link
Collaborator

@leseb leseb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this is the premise of adding more run.yaml into the CRD and i think we should think about how we want to lay out things.

Should we go with:

DistributionServer *DistributionServerSpec

And

type DistributionServerSpec struct {
  Workers  *int32
}

Something like this, i think we need to a way to encapsulate worker into another section?

Thoughts?

EDIT: ok discussed offline:

  • we will revisit this design better when we introduce config properties in the CRD, this will be a new CRD version
  • we just need to make sure the workers match the resource/request

api/v1alpha1/llamastackdistribution_types.go
Distribution DistributionType `json:"distribution"`
ContainerSpec ContainerSpec `json:"containerSpec,omitempty"`
PodOverrides *PodOverrides `json:"podOverrides,omitempty"` // Optional pod-level overrides
// Workers configures the number of uvicorn worker processes to run.
Copy link
Collaborator

@leseb leseb Dec 8, 2025

Choose a reason for hiding this comment

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

Should we link https://fastapi.tiangolo.com/deployment/server-workers/ for more doc, to be better understand the usage?

Copy link
Collaborator Author

@VaishnaviHire VaishnaviHire Dec 9, 2025

Choose a reason for hiding this comment

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

Done

@VaishnaviHire VaishnaviHire force-pushed the add_workers branch 3 times, most recently from 5f7df7e to dafd7d7 Compare December 9, 2025 17:31
@VaishnaviHire
Copy link
Collaborator Author

VaishnaviHire commented Dec 9, 2025

EDIT: ok discussed offline:

  • we will revisit this design better when we introduce config properties in the CRD, this will be a new CRD version
  • we just need to make sure the workers match the resource/request

Updated

leseb
leseb reviewed Dec 10, 2025
View reviewed changes
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

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

one nit thanks for the follow up

controllers/resource_helper.go
// resolveContainerResources ensures the container always has CPU and memory
// requests defined so that HPAs using utilization metrics can function.
func resolveContainerResources(spec llamav1alpha1.ContainerSpec) corev1.ResourceRequirements {
func resolveContainerResources(spec llamav1alpha1.ContainerSpec, workers int32, workersSet bool) corev1.ResourceRequirements {
Copy link
Collaborator

@leseb leseb Dec 10, 2025

Choose a reason for hiding this comment

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

I think we need to log somewhere that we are setting the resources based out of the value of the workers

Copy link
Collaborator Author

@VaishnaviHire VaishnaviHire Dec 10, 2025

Choose a reason for hiding this comment

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

Included it in api docs as well as added a log.

@VaishnaviHire
Copy link
Collaborator Author

VaishnaviHire commented Dec 10, 2025

@mergify rebase

@mergify
Copy link

mergify bot commented Dec 10, 2025

rebase

✅ Branch has been successfully rebased

leseb
leseb reviewed Dec 11, 2025
View reviewed changes
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

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

final nit 🙏🏻

controllers/resource_helper.go Outdated
2) llama stack run /etc/llama-stack/run.yaml ;;
*) echo "Invalid version code: $VERSION_CODE, using new CLI"; llama stack run /etc/llama-stack/run.yaml ;;
2) exec uvicorn llama_stack.core.server.server:create_app --host 0.0.0.0 --port "$PORT" --workers "$WORKERS" --factory ;;
*) exec uvicorn llama_stack.core.server.server:create_app --host 0.0.0.0 --port "$PORT" --workers "$WORKERS" --factory ;;
Copy link
Collaborator

@leseb leseb Dec 11, 2025

Choose a reason for hiding this comment

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

Can we add the log back in here?

Copy link
Collaborator Author

@VaishnaviHire VaishnaviHire Dec 11, 2025

Choose a reason for hiding this comment

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

Np, done

@VaishnaviHire
Copy link
Collaborator Author

VaishnaviHire commented Dec 11, 2025

@mergify rebase

@mergify
Copy link

mergify bot commented Dec 11, 2025

rebase

✅ Branch has been successfully rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leseb leseb leseb requested changes

@mfleader mfleader mfleader approved these changes

@cdoern cdoern Awaiting requested review from cdoern cdoern is a code owner

@derekhiggins derekhiggins Awaiting requested review from derekhiggins derekhiggins is a code owner

@nathan-weinberg nathan-weinberg Awaiting requested review from nathan-weinberg nathan-weinberg is a code owner

@rhdedgar rhdedgar Awaiting requested review from rhdedgar rhdedgar is a code owner

@rhuss rhuss Awaiting requested review from rhuss rhuss is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

v0.5.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add LLS worker capability

3 participants

@VaishnaviHire @leseb @mfleader