feat(spider-py): Add storage functions to get job status and results. - #194
Closed
sitaowang1998 wants to merge 232 commits into
Closed
feat(spider-py): Add storage functions to get job status and results.#194sitaowang1998 wants to merge 232 commits into
sitaowang1998 wants to merge 232 commits into
Conversation
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughAdds a Python client/core/type/storage implementation and tests; introduces uv-based build/lint/test tasks and Python project config; updates CI to log uv and adjusts unit test task; documents new test task names and uv requirement. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as spider.client.Driver
participant Storage as spider.storage.MariaDBStorage
participant DB as MariaDB
User->>Client: submit_jobs(graphs, args)
Client->>Client: Validate counts and arg types
Client->>Client: Build core.TaskGraph inputs
Client->>Storage: submit_jobs(driver_id, task_graphs)
Storage->>DB: Insert jobs/tasks/dependencies/IO
DB-->>Storage: Job IDs
Storage-->>Client: core.Job[]
Client-->>User: Job[]
sequenceDiagram
participant TG1 as core.TaskGraph (parent)
participant TG2 as core.TaskGraph (child)
participant NewTG as core.TaskGraph (chained)
TG1->>TG1: reset_ids()
TG2->>TG2: reset_ids()
TG1->>TG2: Validate outputs vs inputs (count/type)
TG1->>NewTG: Connect outputs→inputs, merge deps/IO
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds Python storage functions for getting job status and results, along with comprehensive unit tests and build configurations. The changes establish the Python client interface for the Spider distributed task execution framework.
- Introduces storage functions for retrieving job status and results in the Python client
- Adds comprehensive unit tests for task graphs, data types, storage backends, and client functionality
- Sets up Python project configuration with pytest, mypy, and ruff for testing and linting
Reviewed Changes
Copilot reviewed 41 out of 43 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| python/src/spider/storage/storage.py | Defines abstract storage interface with methods for job submission, status retrieval, and result fetching |
| python/src/spider/storage/mariadb_storage.py | Implements MariaDB storage backend with concrete implementations of storage interface methods |
| python/tests/storage/test_mariadb.py | Adds unit tests for MariaDB storage functionality including job submission and status checking |
| python/src/spider/client/ | Contains client-side classes for Driver, TaskGraph, Job, and task management |
| python/src/spider/type/ | Implements TDL type system with conversion utilities between Python and TDL types |
| taskfile.yaml, test-tasks.yaml | Updates build configuration to support Python testing and separate C++ vs Python test tasks |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| type=output_type, | ||
| value=core.TaskOutputValue(msgpack.unpackb(data_id)), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Using executemany followed by iterating over cursor is incorrect. executemany doesn't return results for iteration. Use execute in a loop or executemany without expecting results.
Suggested change
| ) | |
| results = [] | |
| for task_id in task_ids: | |
| cursor.execute(GetTaskOutputs, (task_id,)) | |
| for output_type, value, data_id in cursor.fetchall(): | |
| if value is not None: | |
| results.append( | |
| core.TaskOutput( | |
| type=output_type, | |
| value=core.TaskOutputValue(msgpack.unpackb(value)), | |
| ) | |
| ) | |
| if data_id is not None: | |
| results.append( | |
| core.TaskOutput( | |
| type=output_type, | |
| value=core.TaskOutputValue(msgpack.unpackb(data_id)), | |
| ) | |
| ) |
| value=core.TaskOutputValue(msgpack.unpackb(value)), | ||
| ) | ||
| ) | ||
| if data_id is not None: |
There was a problem hiding this comment.
Both value and data_id conditions can be true simultaneously, leading to duplicate results being added. Use elif instead of separate if statements.
Suggested change
| if data_id is not None: | |
| elif data_id is not None: |
|
|
||
| def __init__(self, value: bytes) -> None: | ||
| """Initialize the Data object with the given value.""" | ||
| self.data_id = core.DataId() |
There was a problem hiding this comment.
DataId is an alias for UUID, but calling UUID() without arguments creates a new UUID each time. This should likely be uuid4() to generate a proper UUID.
Suggested change
| self.data_id = core.DataId() | |
| from uuid import uuid4 | |
| class Data: | |
| """Represents a spider client data.""" | |
| def __init__(self, value: bytes) -> None: | |
| """Initialize the Data object with the given value.""" | |
| self.data_id = core.DataId(uuid4()) |
|
|
||
|
|
||
| @pytest.fixture(scope="session") | ||
| def mariadb_storage() -> MariaDBStorage: |
There was a problem hiding this comment.
The hardcoded database credentials (user=spider&password=password) in MariaDBTestUrl pose a security risk. Consider using environment variables or a secure configuration method for test credentials.
| raise StorageError(str(e)) from e | ||
| except msgpack.exceptions.UnpackValueError: | ||
| self._conn.rollback() | ||
| raise |
There was a problem hiding this comment.
The exception is caught but re-raised without providing context about which job or task failed during unpacking, making debugging difficult.
Suggested change
| raise | |
| except msgpack.exceptions.UnpackValueError as e: | |
| self._conn.rollback() | |
| raise StorageError( | |
| f"Failed to unpack task output for job {job.job_id}: {str(e)}" | |
| ) from e |
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.
Description
This PR:
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Documentation
Tests
Chores