From c0c8523ea4e7c68a96ccfc05cb900078701dc3f8 Mon Sep 17 00:00:00 2001 From: Roy Belio Date: Tue, 4 Nov 2025 11:55:06 +0200 Subject: [PATCH] refactor: address PR feedback for llama stack list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- src/llama_stack/cli/stack/list_stacks.py | 10 +- tests/unit/distribution/test_stack_list.py | 145 ++++----------------- 2 files changed, 28 insertions(+), 127 deletions(-) diff --git a/src/llama_stack/cli/stack/list_stacks.py b/src/llama_stack/cli/stack/list_stacks.py index 07e4d825c..ae59ba911 100644 --- a/src/llama_stack/cli/stack/list_stacks.py +++ b/src/llama_stack/cli/stack/list_stacks.py @@ -13,7 +13,7 @@ from llama_stack.core.utils.config_dirs import DISTRIBS_BASE_DIR 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): super().__init__() @@ -30,7 +30,7 @@ class StackListBuilds(Subcommand): """Return a dictionary of distribution names and their paths with source type 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 = {} @@ -41,14 +41,14 @@ class StackListBuilds(Subcommand): 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") - # 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 if DISTRIBS_BASE_DIR.exists(): for stack_dir in DISTRIBS_BASE_DIR.iterdir(): if stack_dir.is_dir() and not stack_dir.name.startswith("."): # Clean up the name (remove llamastack- prefix if present) name = stack_dir.name.replace("llamastack-", "") - distributions[name] = (stack_dir, "built") + distributions[name] = (stack_dir, "custom") return distributions @@ -65,7 +65,7 @@ class StackListBuilds(Subcommand): row = [name, source_type, str(path)] # Check for build and run config files # 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": build_config = "Yes" if (path / "build.yaml").exists() else "No" run_config = "Yes" if (path / "run.yaml").exists() else "No" diff --git a/tests/unit/distribution/test_stack_list.py b/tests/unit/distribution/test_stack_list.py index fa4fe64f9..725ce3410 100644 --- a/tests/unit/distribution/test_stack_list.py +++ b/tests/unit/distribution/test_stack_list.py @@ -4,12 +4,7 @@ # This source code is licensed under the terms described in the LICENSE file in # the root directory of this source tree. -""" -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. -""" +"""Tests for the llama stack list command.""" import argparse from unittest.mock import MagicMock, patch @@ -29,17 +24,17 @@ def list_stacks_command(): @pytest.fixture def mock_distribs_base_dir(tmp_path): - """Create a mock DISTRIBS_BASE_DIR with some built distributions.""" - built_dir = tmp_path / "distributions" - built_dir.mkdir(parents=True, exist_ok=True) + """Create a mock DISTRIBS_BASE_DIR with some custom distributions.""" + custom_dir = tmp_path / "distributions" + custom_dir.mkdir(parents=True, exist_ok=True) - # Create a built distribution - starter_built = built_dir / "starter" - starter_built.mkdir() - (starter_built / "starter-build.yaml").write_text("# build config") - (starter_built / "starter-run.yaml").write_text("# run config") + # Create a custom distribution + starter_custom = custom_dir / "starter" + starter_custom.mkdir() + (starter_custom / "starter-build.yaml").write_text("# build config") + (starter_custom / "starter-run.yaml").write_text("# run config") - return built_dir + return custom_dir @pytest.fixture @@ -61,7 +56,9 @@ def mock_distro_dir(tmp_path): def create_path_mock(builtin_dist_dir): """Create a properly mocked Path object that returns builtin_dist_dir for the distributions path.""" 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.parent.parent.parent = mock_parent_parent_parent @@ -73,14 +70,10 @@ class TestStackList: """Test suite for llama stack list command.""" 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. - - This verifies the fix for issue #3922 where `llama stack list` only showed - distributions after they were run. - """ + """Test that built-in distributions are shown even before running them.""" 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.Path") as mock_path_class: mock_path_class.return_value = mock_path @@ -89,15 +82,17 @@ class TestStackList: # Verify built-in distributions are found 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 assert "starter" in distributions assert "nvidia" 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): - """Test that both built-in and built distributions are shown together.""" + def test_custom_distribution_overrides_builtin(self, list_stacks_command, mock_distro_dir, mock_distribs_base_dir): + """Test that custom 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): @@ -106,105 +101,11 @@ class TestStackList: distributions = list_stacks_command._get_distribution_dirs() - # Should have built-in distributions - builtin_count = sum(1 for _, source_type in distributions.values() if source_type == "built-in") - # 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 + # "starter" should exist and be marked as "custom" (not "built-in") + # because the custom version overrides the built-in one assert "starter" in distributions _, source_type = distributions["starter"] - assert source_type == "built", "Built 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 + assert source_type == "custom", "Custom distribution should override built-in" def test_hidden_directories_ignored(self, list_stacks_command, mock_distro_dir, tmp_path): """Test that hidden directories (starting with .) are ignored."""