phoenix-oss/llama-stack-mirror
0
0
Fork 1
mirror of https://github.com/meta-llama/llama-stack.git synced 2025-12-03 18:00:36 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

refactor: address PR feedback for llama stack list

Browse source 
- Simplify test suite from 8 to 3 core tests for better maintainability
- Change terminology from 'built' to 'custom' for non-built-in distributions
- Make test docstring more general (remove PR-specific reference)

Addresses PR review comments from @cdoern and @leseb

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Roy Belio 2025-11-04 11:55:06 +02:00
parent 650d2fd9e3
commit c0c8523ea4
2 changed files with 28 additions and 127 deletions
Download patch file Download diff file Expand all files Collapse all files

10
src/llama_stack/cli/stack/list_stacks.py
View file

@ -13,7 +13,7 @@ from llama_stack.core.utils.config_dirs import DISTRIBS_BASE_DIR
class StackListBuilds(Subcommand): class StackListBuilds(Subcommand):
"""List available distributions (both built-in and built)""" """List available distributions (both built-in and custom)"""
def __init__(self, subparsers: argparse._SubParsersAction): def __init__(self, subparsers: argparse._SubParsersAction):
super().__init__() super().__init__()
@ -30,7 +30,7 @@ class StackListBuilds(Subcommand):
"""Return a dictionary of distribution names and their paths with source type """Return a dictionary of distribution names and their paths with source type
Returns: Returns:
dict mapping distro name to (path, source_type) where source_type is 'built-in' or 'built' dict mapping distro name to (path, source_type) where source_type is 'built-in' or 'custom'
""" """
distributions = {} distributions = {}
@ -41,14 +41,14 @@ class StackListBuilds(Subcommand):
if stack_dir.is_dir() and not stack_dir.name.startswith(".") and not stack_dir.name.startswith("__"): if stack_dir.is_dir() and not stack_dir.name.startswith(".") and not stack_dir.name.startswith("__"):
distributions[stack_dir.name] = (stack_dir, "built-in") distributions[stack_dir.name] = (stack_dir, "built-in")
# Get built/run distributions from ~/.llama/distributions # Get custom/run distributions from ~/.llama/distributions
# These override built-in ones if they have the same name # These override built-in ones if they have the same name
if DISTRIBS_BASE_DIR.exists(): if DISTRIBS_BASE_DIR.exists():
for stack_dir in DISTRIBS_BASE_DIR.iterdir(): for stack_dir in DISTRIBS_BASE_DIR.iterdir():
if stack_dir.is_dir() and not stack_dir.name.startswith("."): if stack_dir.is_dir() and not stack_dir.name.startswith("."):
# Clean up the name (remove llamastack- prefix if present) # Clean up the name (remove llamastack- prefix if present)
name = stack_dir.name.replace("llamastack-", "") name = stack_dir.name.replace("llamastack-", "")
distributions[name] = (stack_dir, "built") distributions[name] = (stack_dir, "custom")
return distributions return distributions
@ -65,7 +65,7 @@ class StackListBuilds(Subcommand):
row = [name, source_type, str(path)] row = [name, source_type, str(path)]
# Check for build and run config files # Check for build and run config files
# For built-in distributions, configs are named build.yaml and run.yaml # For built-in distributions, configs are named build.yaml and run.yaml
# For built distributions, configs are named {name}-build.yaml and {name}-run.yaml # For custom distributions, configs are named {name}-build.yaml and {name}-run.yaml
if source_type == "built-in": if source_type == "built-in":
build_config = "Yes" if (path / "build.yaml").exists() else "No" build_config = "Yes" if (path / "build.yaml").exists() else "No"
run_config = "Yes" if (path / "run.yaml").exists() else "No" run_config = "Yes" if (path / "run.yaml").exists() else "No"

145
tests/unit/distribution/test_stack_list.py
View file

@ -4,12 +4,7 @@
# This source code is licensed under the terms described in the LICENSE file in # This source code is licensed under the terms described in the LICENSE file in
# the root directory of this source tree. # the root directory of this source tree.
""" """Tests for the llama stack list command."""
Tests for the llama stack list command.
These tests verify the fix for issue #3922 where `llama stack list` only showed
distributions after they were run.
"""
import argparse import argparse
from unittest.mock import MagicMock, patch from unittest.mock import MagicMock, patch
@ -29,17 +24,17 @@ def list_stacks_command():
@pytest.fixture @pytest.fixture
def mock_distribs_base_dir(tmp_path): def mock_distribs_base_dir(tmp_path):
"""Create a mock DISTRIBS_BASE_DIR with some built distributions.""" """Create a mock DISTRIBS_BASE_DIR with some custom distributions."""
built_dir = tmp_path / "distributions" custom_dir = tmp_path / "distributions"
built_dir.mkdir(parents=True, exist_ok=True) custom_dir.mkdir(parents=True, exist_ok=True)
# Create a built distribution # Create a custom distribution
starter_built = built_dir / "starter" starter_custom = custom_dir / "starter"
starter_built.mkdir() starter_custom.mkdir()
(starter_built / "starter-build.yaml").write_text("# build config") (starter_custom / "starter-build.yaml").write_text("# build config")
(starter_built / "starter-run.yaml").write_text("# run config") (starter_custom / "starter-run.yaml").write_text("# run config")
return built_dir return custom_dir
@pytest.fixture @pytest.fixture
@ -61,7 +56,9 @@ def mock_distro_dir(tmp_path):
def create_path_mock(builtin_dist_dir): def create_path_mock(builtin_dist_dir):
"""Create a properly mocked Path object that returns builtin_dist_dir for the distributions path.""" """Create a properly mocked Path object that returns builtin_dist_dir for the distributions path."""
mock_parent_parent_parent = MagicMock() mock_parent_parent_parent = MagicMock()
mock_parent_parent_parent.__truediv__ = lambda self, other: builtin_dist_dir if other == "distributions" else MagicMock() mock_parent_parent_parent.__truediv__ = (
lambda self, other: builtin_dist_dir if other == "distributions" else MagicMock()
)
mock_path = MagicMock() mock_path = MagicMock()
mock_path.parent.parent.parent = mock_parent_parent_parent mock_path.parent.parent.parent = mock_parent_parent_parent
@ -73,14 +70,10 @@ class TestStackList:
"""Test suite for llama stack list command.""" """Test suite for llama stack list command."""
def test_builtin_distros_shown_without_running(self, list_stacks_command, mock_distro_dir, tmp_path): def test_builtin_distros_shown_without_running(self, list_stacks_command, mock_distro_dir, tmp_path):
"""Test that built-in distributions are shown even before running them. """Test that built-in distributions are shown even before running them."""
This verifies the fix for issue #3922 where `llama stack list` only showed
distributions after they were run.
"""
mock_path = create_path_mock(mock_distro_dir) mock_path = create_path_mock(mock_distro_dir)
# Mock DISTRIBS_BASE_DIR to be a non-existent directory (no built distributions) # Mock DISTRIBS_BASE_DIR to be a non-existent directory (no custom distributions)
with patch("llama_stack.cli.stack.list_stacks.DISTRIBS_BASE_DIR", tmp_path / "nonexistent"): with patch("llama_stack.cli.stack.list_stacks.DISTRIBS_BASE_DIR", tmp_path / "nonexistent"):
with patch("llama_stack.cli.stack.list_stacks.Path") as mock_path_class: with patch("llama_stack.cli.stack.list_stacks.Path") as mock_path_class:
mock_path_class.return_value = mock_path mock_path_class.return_value = mock_path
@ -89,15 +82,17 @@ class TestStackList:
# Verify built-in distributions are found # Verify built-in distributions are found
assert len(distributions) > 0, "Should find built-in distributions" assert len(distributions) > 0, "Should find built-in distributions"
assert all(source_type == "built-in" for _, source_type in distributions.values()), "All should be built-in" assert all(source_type == "built-in" for _, source_type in distributions.values()), (
"All should be built-in"
)
# Check specific distributions we created # Check specific distributions we created
assert "starter" in distributions assert "starter" in distributions
assert "nvidia" in distributions assert "nvidia" in distributions
assert "dell" in distributions assert "dell" in distributions
def test_builtin_and_built_distros_shown_together(self, list_stacks_command, mock_distro_dir, mock_distribs_base_dir): def test_custom_distribution_overrides_builtin(self, list_stacks_command, mock_distro_dir, mock_distribs_base_dir):
"""Test that both built-in and built distributions are shown together.""" """Test that custom distributions override built-in ones with the same name."""
mock_path = create_path_mock(mock_distro_dir) mock_path = create_path_mock(mock_distro_dir)
with patch("llama_stack.cli.stack.list_stacks.DISTRIBS_BASE_DIR", mock_distribs_base_dir): with patch("llama_stack.cli.stack.list_stacks.DISTRIBS_BASE_DIR", mock_distribs_base_dir):
@ -106,105 +101,11 @@ class TestStackList:
distributions = list_stacks_command._get_distribution_dirs() distributions = list_stacks_command._get_distribution_dirs()
# Should have built-in distributions # "starter" should exist and be marked as "custom" (not "built-in")
builtin_count = sum(1 for _, source_type in distributions.values() if source_type == "built-in") # because the custom version overrides the built-in one
# Should have built distributions
built_count = sum(1 for _, source_type in distributions.values() if source_type == "built")
assert builtin_count > 0, "Should have built-in distributions"
assert built_count > 0, "Should have built distributions"
def test_built_distribution_overrides_builtin(self, list_stacks_command, mock_distro_dir, mock_distribs_base_dir):
"""Test that built distributions override built-in ones with the same name."""
mock_path = create_path_mock(mock_distro_dir)
with patch("llama_stack.cli.stack.list_stacks.DISTRIBS_BASE_DIR", mock_distribs_base_dir):
with patch("llama_stack.cli.stack.list_stacks.Path") as mock_path_class:
mock_path_class.return_value = mock_path
distributions = list_stacks_command._get_distribution_dirs()
# "starter" should exist and be marked as "built" (not "built-in")
# because the built version overrides the built-in one
assert "starter" in distributions assert "starter" in distributions
_, source_type = distributions["starter"] _, source_type = distributions["starter"]
assert source_type == "built", "Built distribution should override built-in" assert source_type == "custom", "Custom distribution should override built-in"
def test_empty_distributions(self, list_stacks_command, tmp_path):
"""Test behavior when no distributions exist."""
nonexistent = tmp_path / "nonexistent"
mock_path = create_path_mock(nonexistent)
with patch("llama_stack.cli.stack.list_stacks.DISTRIBS_BASE_DIR", nonexistent):
with patch("llama_stack.cli.stack.list_stacks.Path") as mock_path_class:
mock_path_class.return_value = mock_path
distributions = list_stacks_command._get_distribution_dirs()
assert len(distributions) == 0, "Should return empty dict when no distributions exist"
def test_config_files_detection_builtin(self, list_stacks_command, mock_distro_dir, tmp_path):
"""Test that config files are correctly detected for built-in distributions."""
mock_path = create_path_mock(mock_distro_dir)
with patch("llama_stack.cli.stack.list_stacks.DISTRIBS_BASE_DIR", tmp_path / "nonexistent"):
with patch("llama_stack.cli.stack.list_stacks.Path") as mock_path_class:
mock_path_class.return_value = mock_path
distributions = list_stacks_command._get_distribution_dirs()
# Check that starter has both config files
if "starter" in distributions:
path, source_type = distributions["starter"]
if source_type == "built-in":
assert (path / "build.yaml").exists()
assert (path / "run.yaml").exists()
def test_config_files_detection_built(self, list_stacks_command, tmp_path):
"""Test that config files are correctly detected for built distributions."""
# Create a built distribution
built_dir = tmp_path / "distributions"
built_dir.mkdir(parents=True)
test_distro = built_dir / "test-distro"
test_distro.mkdir()
(test_distro / "test-distro-build.yaml").write_text("# build")
(test_distro / "test-distro-run.yaml").write_text("# run")
nonexistent = tmp_path / "nonexistent"
mock_path = create_path_mock(nonexistent)
with patch("llama_stack.cli.stack.list_stacks.DISTRIBS_BASE_DIR", built_dir):
with patch("llama_stack.cli.stack.list_stacks.Path") as mock_path_class:
mock_path_class.return_value = mock_path
distributions = list_stacks_command._get_distribution_dirs()
assert "test-distro" in distributions
path, source_type = distributions["test-distro"]
assert source_type == "built"
assert (path / "test-distro-build.yaml").exists()
assert (path / "test-distro-run.yaml").exists()
def test_llamastack_prefix_stripped(self, list_stacks_command, tmp_path):
"""Test that llamastack- prefix is stripped from built distribution names."""
# Create a built distribution with llamastack- prefix
built_dir = tmp_path / "distributions"
built_dir.mkdir(parents=True)
distro_with_prefix = built_dir / "llamastack-mystack"
distro_with_prefix.mkdir()
nonexistent = tmp_path / "nonexistent"
mock_path = create_path_mock(nonexistent)
with patch("llama_stack.cli.stack.list_stacks.DISTRIBS_BASE_DIR", built_dir):
with patch("llama_stack.cli.stack.list_stacks.Path") as mock_path_class:
mock_path_class.return_value = mock_path
distributions = list_stacks_command._get_distribution_dirs()
# Should be listed as "mystack" not "llamastack-mystack"
assert "mystack" in distributions
assert "llamastack-mystack" not in distributions
def test_hidden_directories_ignored(self, list_stacks_command, mock_distro_dir, tmp_path): def test_hidden_directories_ignored(self, list_stacks_command, mock_distro_dir, tmp_path):
"""Test that hidden directories (starting with .) are ignored.""" """Test that hidden directories (starting with .) are ignored."""