Unit test for swarm #36
Conversation
- Add detailed logging in swarm workflows to track task lifecycle - Log swarm state (running/finished/failed/queued counts) at each step - Add null checks in HatchetInvoker.run_success/run_error to prevent crashes - Log missing tasks in fill_running_tasks when BatchItemTaskSignature not found - Add logger parameter to fill_running_tasks for Hatchet context logging - Log why swarm is not done when tasks are still pending - Better error messages with task IDs for debugging These changes help diagnose issues where tasks disappear from swarms without being marked as finished or failed.
There was a problem hiding this comment.
Pull request overview
This pull request focuses on code quality improvements including import reordering, removal of unused imports, enhanced logging for swarm workflows, and improved error handling for missing tasks in Redis. The changes also include GitHub workflow adjustments.
Changes:
- Reorganized imports across test files to follow PEP 8 style (standard library → third-party → local imports)
- Added comprehensive logging and error handling for swarm task lifecycle operations
- Implemented null-safety checks for tasks that may have been removed from Redis before callbacks are triggered
- Modified GitHub workflow configurations for CI and release processes
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_sign.py | Reordered imports to follow PEP 8 conventions |
| tests/unit/test_client.py | Reordered imports to follow PEP 8 conventions |
| tests/unit/swarm/test_config.py | Reordered imports to follow PEP 8 conventions |
| tests/unit/swarm/test_call_item.py | Reordered imports to follow PEP 8 conventions |
| tests/unit/swarm/conftest.py | Removed unused imports (mageflow, ChainTaskSignature) |
| tests/unit/complex/test_chain_and_swarm.py | Reordered imports to follow PEP 8 conventions |
| tests/unit/chain/test_status_changes.py | Reordered imports to follow PEP 8 conventions |
| tests/unit/chain/test_chain.py | Reordered imports to follow PEP 8 conventions |
| tests/integration/test_redis_ttl.py | Reordered imports and cleaned up whitespace |
| tests/integration/hatchet/test_task_models.py | Removed unused pytest_asyncio import |
| tests/integration/hatchet/test_complex_scerios.py | Removed blank line |
| tests/integration/hatchet/swarm/test_stop_resume.py | Reordered imports to follow PEP 8 conventions |
| tests/integration/hatchet/swarm/test_edge_cases.py | Reordered imports to follow PEP 8 conventions |
| tests/integration/hatchet/swarm/test__swarm.py | Reordered imports and removed unused import (is_wf_done) |
| tests/integration/hatchet/signature/test_stop_resume.py | Reordered imports to follow PEP 8 conventions |
| tests/integration/hatchet/signature/test_edge_case.py | Reordered imports to follow PEP 8 conventions |
| tests/integration/hatchet/signature/test__signature.py | Reordered imports to follow PEP 8 conventions |
| tests/integration/hatchet/models.py | Reordered imports to follow PEP 8 conventions |
| tests/integration/hatchet/conftest.py | Reordered imports to follow PEP 8 conventions |
| tests/integration/hatchet/chain/test_edge_cases.py | Reordered imports to follow PEP 8 conventions |
| tests/integration/hatchet/chain/test__chain.py | Reordered imports to follow PEP 8 conventions |
| tests/integration/hatchet/assertions.py | Removed blank line |
| mageflow/swarm/workflows.py | Added extensive logging and null-safety checks for swarm task lifecycle operations |
| mageflow/swarm/model.py | Added logger parameter and enhanced error handling for missing tasks in fill_running_tasks |
| mageflow/invokers/hatchet.py | Added null-safety checks with logging for tasks missing from Redis during callbacks |
| .github/workflows/release.yml | Changed job trigger condition from needs dependency to conditional check |
| .github/workflows/ci.yml | Renamed workflow from "Python tests" to "CI" and removed workflow_dispatch trigger |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(tasks) != len(publish_coroutine): | ||
| raise MissingSwarmItemError(f"swarm item was deleted before swarm is done") | ||
| return len(tasks) | ||
| raise MissingSwarmItemError( | ||
| f"swarm item was deleted before swarm is done. " | ||
| f"Missing: {missing_task_ids}" | ||
| ) |
There was a problem hiding this comment.
The error check is comparing the wrong variables. The condition checks if len(tasks) differs from len(publish_coroutine), but publish_coroutine is built from valid_tasks, not tasks. This check will always pass even when tasks are missing. The condition should compare len(valid_tasks) with len(publish_coroutine) (which should always be equal), or better yet, compare len(tasks) with len(valid_tasks) to detect missing tasks.
| import logging | ||
| logging.warning( |
There was a problem hiding this comment.
The logging module is imported inside the function body rather than at the module level. This is inefficient as the import statement will be executed every time the function is called when a task is not found. Move the import statement to the top of the file with other imports.
| import logging | ||
| logging.warning( |
There was a problem hiding this comment.
The logging module is imported inside the function body rather than at the module level. This is inefficient as the import statement will be executed every time the function is called when a task is not found. Move the import statement to the top of the file with other imports.
| jobs: | ||
| create-github-release: | ||
| needs: check-tests | ||
| if: github.ref_type == 'tag' && github.event.base_ref == 'refs/heads/main' |
There was a problem hiding this comment.
The condition github.event.base_ref == 'refs/heads/main' will not work as expected for tag push events. When a tag is pushed, github.event.base_ref is typically empty or undefined. This condition should likely check that the tag was created from the main branch using a different approach, such as checking out the repository and verifying the tag points to a commit on main, or removing this condition entirely if tags should only be created from main by policy.
| if: github.ref_type == 'tag' && github.event.base_ref == 'refs/heads/main' | |
| if: github.ref_type == 'tag' |
| from threading import Thread | ||
| from typing import Generator, Callable, AsyncGenerator | ||
|
|
||
| import mageflow |
There was a problem hiding this comment.
Module 'mageflow' is imported with both 'import' and 'import from'.
| import mageflow |
No description provided.