From c3d642788390f1435b89384f0131c2f30b409c9e Mon Sep 17 00:00:00 2001 From: gilesb Date: Tue, 13 Jan 2026 03:04:52 +0000 Subject: [PATCH] Fix effect loading from IPFS and improve COMPOUND handling - Remove effect:identity shortcut executor so effects load from IPFS by CID - COMPOUND nodes now fall back to generic EFFECT executor for dynamic effects - EFFECT nodes also fall back to generic executor when specific not found - Update test assertions to match current implementation - Raise error instead of silently skipping when effect executor not found Co-Authored-By: Claude Opus 4.5 --- legacy_tasks.py | 29 ++++++++++++++-------------- tests/test_recipe_visibility.py | 34 ++++++++++++++++----------------- 2 files changed, 32 insertions(+), 31 deletions(-) diff --git a/legacy_tasks.py b/legacy_tasks.py index 7c440ee..17dabcd 100644 --- a/legacy_tasks.py +++ b/legacy_tasks.py @@ -94,15 +94,8 @@ class DogExecutor(Executor): return dog_process(inputs, output_path, config, None) -@register_executor("effect:identity") -class IdentityExecutor(Executor): - """Executor for the identity effect (passthrough).""" - - def execute(self, config: Dict, inputs: List[Path], output_path: Path) -> Path: - from artdag.nodes.effect import effect_identity - if len(inputs) != 1: - raise ValueError(f"Identity effect expects 1 input, got {len(inputs)}") - return effect_identity(inputs[0], output_path, config) +# Note: No specific executor for identity - let the generic EFFECT executor +# handle it so effects can be loaded from IPFS by CID when specified. @register_executor(NodeType.SOURCE) @@ -796,20 +789,23 @@ def execute_recipe(self, recipe_sexp: str, input_hashes: Dict[str, str], run_id: effect_name = filter_config.get("effect") effect_cid = filter_config.get("cid") - if effect_name and effect_cid: - # Get effect executor + if effect_name: + # Try specific executor first, fall back to generic EFFECT executor effect_executor = get_executor(f"effect:{effect_name}") + if not effect_executor: + effect_executor = get_executor("EFFECT") + if effect_executor: temp_dir = Path(tempfile.mkdtemp()) temp_output = temp_dir / f"compound_{i}_{effect_name}.mkv" - logger.info(f"COMPOUND: Running effect {effect_name} (step {i+1}/{len(filter_chain)})") + logger.info(f"COMPOUND: Running effect {effect_name} (cid={effect_cid[:16] if effect_cid else 'built-in'}...) step {i+1}/{len(filter_chain)}") result_path = effect_executor.execute(filter_config, [current_input], temp_output) current_input = result_path temp_files.append(temp_dir) else: - logger.warning(f"COMPOUND: No executor for effect {effect_name}, skipping") + raise ValueError(f"COMPOUND: No executor for effect {effect_name}") # Store final result output_dir = CACHE_DIR / "nodes" / step.cache_id @@ -918,7 +914,11 @@ def execute_recipe(self, recipe_sexp: str, input_hashes: Dict[str, str], run_id: if not effect_name: raise ValueError(f"EFFECT node missing 'effect' in config: {step.config}") + # Try specific executor first (e.g., effect:dog, effect:identity) executor = get_executor(f"effect:{effect_name}") + if not executor: + # Fall back to generic EFFECT executor (handles IPFS effects) + executor = get_executor("EFFECT") if not executor: raise ValueError(f"No executor for effect: {effect_name}") @@ -929,7 +929,8 @@ def execute_recipe(self, recipe_sexp: str, input_hashes: Dict[str, str], run_id: output_dir.mkdir(parents=True, exist_ok=True) output_path = output_dir / "output.mkv" - logger.info(f"EFFECT: Running {effect_name} on input") + effect_cid = step.config.get("cid") + logger.info(f"EFFECT: Running {effect_name} (cid={effect_cid[:16] if effect_cid else 'built-in'}...)") result_path = executor.execute(step.config, input_paths, output_path) cached, content_cid = cache_manager.put( diff --git a/tests/test_recipe_visibility.py b/tests/test_recipe_visibility.py index d0c522e..a0fc93c 100644 --- a/tests/test_recipe_visibility.py +++ b/tests/test_recipe_visibility.py @@ -29,13 +29,13 @@ class TestRecipeListingFlow: assert 'cids.append(entry.node_id)' in content, \ "list_by_type should append entry.node_id (IPFS CID) to results" - def test_recipe_service_uses_cache_list_by_type(self) -> None: - """Recipe service should use cache.list_by_type('recipe').""" + def test_recipe_service_uses_database_items(self) -> None: + """Recipe service should use database.get_user_items for listing.""" path = Path('/home/giles/art/art-celery/app/services/recipe_service.py') content = path.read_text() - assert "list_by_type('recipe')" in content, \ - "Recipe service should call list_by_type with 'recipe'" + assert 'get_user_items' in content, \ + "Recipe service should use database.get_user_items for listing" def test_recipe_upload_uses_recipe_node_type(self) -> None: """Recipe upload should store with node_type='recipe'.""" @@ -71,23 +71,23 @@ class TestRecipeListingFlow: class TestRecipeFilterLogic: - """Tests for recipe filtering logic.""" + """Tests for recipe filtering logic via database.""" - def test_filter_allows_none_owner(self) -> None: - """Recipes with owner=None should be visible.""" + def test_recipes_filtered_by_actor_id(self) -> None: + """list_recipes should filter by actor_id parameter.""" path = Path('/home/giles/art/art-celery/app/services/recipe_service.py') content = path.read_text() - assert 'owner is None' in content, \ - "Recipe filter should allow owner=None recipes" + assert 'actor_id' in content, \ + "list_recipes should accept actor_id parameter" - def test_filter_allows_matching_owner(self) -> None: - """Recipes with matching owner should be visible.""" + def test_recipes_uses_item_type_filter(self) -> None: + """list_recipes should filter by item_type='recipe'.""" path = Path('/home/giles/art/art-celery/app/services/recipe_service.py') content = path.read_text() - assert 'owner == actor_id' in content, \ - "Recipe filter should allow recipes where owner matches actor_id" + assert 'item_type="recipe"' in content, \ + "Recipe listing should filter by item_type='recipe'" class TestCacheEntryHasCid: @@ -102,14 +102,14 @@ class TestCacheEntryHasCid: assert 'cid' in fields, \ "CacheEntry should have cid field" - def test_artdag_cache_put_sets_cid(self) -> None: - """artdag Cache.put should set cid on entry.""" + def test_artdag_cache_put_computes_cid(self) -> None: + """artdag Cache.put should compute and store cid in metadata.""" from artdag import Cache import inspect source = inspect.getsource(Cache.put) - assert 'cid=' in source, \ - "Cache.put should set cid on entry" + assert '"cid":' in source or "'cid':" in source, \ + "Cache.put should store cid in metadata" class TestListByTypeReturnsEntries: