Draft
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpgrade project to llama-stack 0.4.x and the new llama-stack-client API, updating dataset/benchmark client namespaces, demos, tests, CI matrix, and dependencies accordingly. Sequence diagram for updated dataset and benchmark registration via llama-stack-client 0.4.xsequenceDiagram
actor User
participant BasicDemoNotebook as BasicDemo_notebook
participant LlamaStackClient as LlamaStackClient
participant BetaNamespace as beta
participant AlphaNamespace as alpha
participant DatasetAPI as datasets
participant BenchmarkAPI as benchmarks
participant LlamaStackService as LlamaStack_server
User->>BasicDemoNotebook: Run demo cells
BasicDemoNotebook->>LlamaStackClient: init(base_url)
BasicDemoNotebook->>LlamaStackClient: models.list()
LlamaStackClient-->>BasicDemoNotebook: available_models
Note over BasicDemoNotebook,LlamaStackClient: Dataset registration now uses client.beta.datasets
BasicDemoNotebook->>BetaNamespace: client.beta
BetaNamespace->>DatasetAPI: register(dataset_id, purpose, source, metadata)
DatasetAPI->>LlamaStackService: POST /beta/datasets/register
LlamaStackService-->>DatasetAPI: dataset_response
DatasetAPI-->>BasicDemoNotebook: dataset_response
Note over BasicDemoNotebook,LlamaStackClient: Benchmark registration now uses client.alpha.benchmarks
BasicDemoNotebook->>AlphaNamespace: client.alpha
AlphaNamespace->>BenchmarkAPI: register(benchmark_id, dataset_id, scoring_functions, provider_id)
BenchmarkAPI->>LlamaStackService: POST /alpha/benchmarks/register
LlamaStackService-->>BenchmarkAPI: benchmark_response
BenchmarkAPI-->>BasicDemoNotebook: benchmark_response
Note over BasicDemoNotebook,LlamaStackClient: Dataset cleanup now uses beta namespace
BasicDemoNotebook->>BetaNamespace: datasets.unregister(dataset_id)
BetaNamespace->>DatasetAPI: unregister(dataset_id)
DatasetAPI->>LlamaStackService: DELETE /beta/datasets/{dataset_id}
LlamaStackService-->>DatasetAPI: success
DatasetAPI-->>BasicDemoNotebook: success
Sequence diagram for Kubeflow component using beta.datasets.retrievesequenceDiagram
participant KubeflowPipeline as Kubeflow_pipeline
participant RetrieveComponent as retrieve_data_from_llama_stack
participant LlamaStackClient as LlamaStackClient
participant BetaNamespace as beta
participant DatasetAPI as datasets
participant LlamaStackService as LlamaStack_server
participant OutputArtifact as output_dataset
KubeflowPipeline->>RetrieveComponent: invoke(llama_stack_base_url, dataset_id, output_dataset)
RetrieveComponent->>LlamaStackClient: init(base_url=llama_stack_base_url)
Note over RetrieveComponent,LlamaStackClient: Dataset retrieval now uses client.beta.datasets
RetrieveComponent->>BetaNamespace: client.beta
BetaNamespace->>DatasetAPI: retrieve(dataset_id)
DatasetAPI->>LlamaStackService: GET /beta/datasets/{dataset_id}
LlamaStackService-->>DatasetAPI: dataset(source.rows)
DatasetAPI-->>RetrieveComponent: dataset
RetrieveComponent->>RetrieveComponent: df = DataFrame(dataset.source.rows)
RetrieveComponent->>OutputArtifact: df.to_json(path, records, lines=True)
OutputArtifact-->>KubeflowPipeline: JSON dataset artifact
Class diagram for updated llama-stack-client 0.4.x namespace usageclassDiagram
class LlamaStackClient {
+str base_url
+ModelsNamespace models
+BetaNamespace beta
+AlphaNamespace alpha
+__init__(base_url)
}
class ModelsNamespace {
+list list()
}
class BetaNamespace {
+DatasetsNamespace datasets
}
class AlphaNamespace {
+BenchmarksNamespace benchmarks
}
class DatasetsNamespace {
+register(dataset_id, purpose, source, metadata)
+unregister(dataset_id)
+retrieve(dataset_id)
}
class BenchmarksNamespace {
+register(benchmark_id, dataset_id, scoring_functions, provider_id)
}
LlamaStackClient --> ModelsNamespace : has
LlamaStackClient --> BetaNamespace : has
LlamaStackClient --> AlphaNamespace : has
BetaNamespace --> DatasetsNamespace : has
AlphaNamespace --> BenchmarksNamespace : has
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
basic_demo.ipynb, replacing the simple model-type assertions with justpprint(available_models)removes a quick sanity check; consider keeping a minimal validation (e.g., assert at least one LLM model exists) alongside the debug print to preserve early failure behavior for users following the demo. - Now that datasets and benchmarks are accessed under
beta/alphanamespaces in several places, it would be good to double-check that all usages across the repository (including any less-visible scripts or utilities) are consistently updated to avoid mixing legacy and new client APIs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `basic_demo.ipynb`, replacing the simple model-type assertions with just `pprint(available_models)` removes a quick sanity check; consider keeping a minimal validation (e.g., assert at least one LLM model exists) alongside the debug print to preserve early failure behavior for users following the demo.
- Now that datasets and benchmarks are accessed under `beta`/`alpha` namespaces in several places, it would be good to double-check that all usages across the repository (including any less-visible scripts or utilities) are consistently updated to avoid mixing legacy and new client APIs.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
9589001 to
cf20dcf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary