-
Notifications
You must be signed in to change notification settings - Fork 52
LCORE-331: Azure inference supported #654
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
LCORE-331: Azure inference supported #654
Conversation
WalkthroughAdds Azure as an explicit E2E/stack target: CI workflow gains an Azure matrix entry and token acquisition step, docker-compose forwards AZURE_API_KEY to services, README and provider docs updated for Azure, and two new Azure-focused stack configs are added (example and E2E). (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant AAD as Azure AD
participant DC as Docker Compose
participant Svc as Services (llama-stack / lightspeed-stack)
participant T as E2E Tests
Dev->>GH: Push/PR triggers E2E (matrix: ci, azure)
alt environment == "azure"
GH->>AAD: OAuth2 client_credentials (CLIENT_ID, TENANT_ID, CLIENT_SECRET)
AAD-->>GH: JSON { access_token }
GH->>GH: Extract & export AZURE_API_KEY
else environment != "azure"
GH->>GH: Skip Azure token step
end
GH->>DC: docker-compose up (env includes AZURE_API_KEY if set)
DC->>Svc: Start containers with env vars
Svc-->>T: Run E2E tests (inference may call Azure with AZURE_API_KEY)
T-->>GH: Results (pass/fail)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docker-compose.yaml (1)
15-15: Pass through AZURE_API_TYPE for Azure auth mode parityIf you use AAD tokens in CI (Bearer) vs API keys (api-key), pass AZURE_API_TYPE through compose as well so llama-stack can switch modes.
services: llama-stack: environment: - OPENAI_API_KEY=${OPENAI_API_KEY} - AZURE_API_KEY=${AZURE_API_KEY} + - AZURE_API_TYPE=${AZURE_API_TYPE:-} ... lightspeed-stack: environment: - OPENAI_API_KEY=${OPENAI_API_KEY} - AZURE_API_KEY=${AZURE_API_KEY} + - AZURE_API_TYPE=${AZURE_API_TYPE:-}Also applies to: 40-40
examples/azure-run.yaml (1)
72-79: Fix env substitution and generalize api_base in exampleUse supported env syntax and avoid hard‑coded API base in examples.
config: api_key: ${env.AZURE_API_KEY} - api_base: https://ols-test.openai.azure.com/ + api_base: ${env.AZURE_API_BASE} api_version: 2024-02-15-preview - api_type: ${env.AZURE_API_TYPE:=} + api_type: ${env.AZURE_API_TYPE}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/e2e_tests.yaml(2 hunks)README.md(1 hunks)docker-compose.yaml(2 hunks)docs/providers.md(2 hunks)examples/azure-run.yaml(1 hunks)tests/e2e/configs/run-azure.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (5)
docs/providers.md (1)
39-39: Azure provider doc updates look goodAzure marked supported and link updated. Please verify no extra pip deps are needed for the azure remote in upstream llama-stack.
Also applies to: 290-290
README.md (1)
128-130: Azure rows added to compatibility table look goodAligns with new example and tests.
tests/e2e/configs/run-azure.yaml (1)
125-128: Confirm Azure model/deployment mappingAzure OpenAI often requires a deployment name rather than a raw model id. Confirm llama‑stack’s Azure provider expects provider_model_id=gpt-4o-mini or a deployment identifier; adjust if needed.
examples/azure-run.yaml (1)
125-128: Verify deployment vs model idAs above, confirm whether provider_model_id should be the Azure deployment name rather than the model string.
.github/workflows/e2e_tests.yaml (1)
79-104: Export AZURE_API_TYPE for AAD/Bearer auth
Add immediately after setting AZURE_API_KEY:echo "AZURE_API_KEY=$ACCESS_TOKEN" >> $GITHUB_ENV +echo "AZURE_API_TYPE=azure_ad" >> $GITHUB_ENVVerify that the Azure provider reads
api_typefrom the environment (and acceptsazure_ad); adjust the key or value if needed.
| CLIENT_SECRET: ${{ secrets.CLIENT_SECRET }} | ||
| CLIENT_ID: ${{ secrets.CLIENT_ID }} | ||
| TENANT_ID: ${{ secrets.TENAND_ID }} | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix secret name and restrict secrets to the Azure step (avoid broad exposure)
- Typo: TENAND_ID → TENANT_ID.
- Don’t export CLIENT_ID/CLIENT_SECRET/TENANT_ID at job scope; they leak to all steps (incl. third‑party actions). Keep them only in the Azure token step where they’re used.
env:
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
- CLIENT_SECRET: ${{ secrets.CLIENT_SECRET }}
- CLIENT_ID: ${{ secrets.CLIENT_ID }}
- TENANT_ID: ${{ secrets.TENAND_ID }}They are already correctly provided again via the Azure token step’s env (keep that).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CLIENT_SECRET: ${{ secrets.CLIENT_SECRET }} | |
| CLIENT_ID: ${{ secrets.CLIENT_ID }} | |
| TENANT_ID: ${{ secrets.TENAND_ID }} | |
| env: | |
| OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} |
🤖 Prompt for AI Agents
.github/workflows/e2e_tests.yaml around lines 14 to 17: the job-level env has a
typo and exposes sensitive secrets broadly; change TENAND_ID to TENANT_ID and
remove CLIENT_SECRET, CLIENT_ID and TENANT_ID from the job-level env block so
they are not exported to all steps, leaving those secrets only in the specific
Azure token step’s env where they are already provided.
| - provider_id: azure | ||
| provider_type: remote::azure | ||
| config: | ||
| api_key: ${env.AZURE_API_KEY} | ||
| api_base: https://ols-test.openai.azure.com/ | ||
| api_version: 2024-02-15-preview | ||
| api_type: ${env.AZURE_API_TYPE:=} | ||
| post_training: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid env default syntax; parameterize api_base
- llama-stack env substitution supports ${env.VAR}; the default form ${env.VAR:=} is likely unsupported and may be passed as a literal.
config:
api_key: ${env.AZURE_API_KEY}
- api_base: https://ols-test.openai.azure.com/
+ api_base: ${env.AZURE_API_BASE}
api_version: 2024-02-15-preview
- api_type: ${env.AZURE_API_TYPE:=}
+ api_type: ${env.AZURE_API_TYPE}Set AZURE_API_BASE and (optionally) AZURE_API_TYPE in the environment (compose/workflow).
🤖 Prompt for AI Agents
In tests/e2e/configs/run-azure.yaml around lines 72 to 79, the file uses
unsupported env default syntax (${env.AZURE_API_TYPE:=}) and hardcodes api_base;
change api_base to use an environment variable (e.g. api_base:
${env.AZURE_API_BASE}) and replace the unsupported default form with a plain env
substitution (api_type: ${env.AZURE_API_TYPE}) or remove the api_type line if
optional, then ensure AZURE_API_BASE (and AZURE_API_TYPE if used) are set in the
environment/compose or workflow.
c26dffd to
fcf2306
Compare
radofuchs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tisnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Azure inference supported
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Tests
Chores