feat(spider-py): Add Data, Task, and TaskGraph to core. - #180
Merged
Conversation
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
python/pyproject.toml (1)
1-10: Consider marking package as private to prevent accidental publicationPer prior team practice with uv, add the “Private :: Do Not Upload” Trove classifier if this package isn’t meant for PyPI.
[project] name = "spider" version = "0.0.1" description = "Spider is a distributed task execution framework" readme = "README.md" requires-python = ">=3.10" dependencies = [ "mariadb>=1.1.13", "msgpack-types>=0.5.0", ] +classifiers = [ + "Private :: Do Not Upload", +]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
python/pyproject.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: sitaowang1998
PR: y-scope/spider#180
File: python/src/core/taskgraph.py:50-56
Timestamp: 2025-08-06T13:45:33.914Z
Learning: In the TaskGraph class in python/src/core/taskgraph.py, there is a program invariant that ensures all task IDs in TaskGraph.dependencies are guaranteed to exist in TaskGraph.tasks. The add_task method enforces this by requiring parent and children tasks to already be present in the graph before adding dependencies, eliminating the need for error handling in get_parents and get_children methods.
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: build-tasks.yaml:21-45
Timestamp: 2025-08-03T01:49:27.764Z
Learning: In the spider project, external tools like `uv`, Python, and Task are documented as requirements in README.md and are expected to be pre-installed by users rather than bootstrapped by the build system.
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: python/spider/client/pyproject.toml:6-7
Timestamp: 2025-08-03T01:52:26.847Z
Learning: In the spider project, internal dependencies between packages (like spider-client depending on spider-core) are managed without version pinning since they are developed together in the same repository and versions are kept synchronized.
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: build-tasks.yaml:26-29
Timestamp: 2025-08-03T01:54:03.099Z
Learning: In Task build systems, glob patterns like "python/**/*" properly match files at any nested level, including package-level pyproject.toml files in subdirectories like "python/spider/client/pyproject.toml". The "**" wildcard recursively matches any number of directory levels.
Learnt from: davidlion
PR: y-scope/spider#100
File: src/spider/worker/worker.cpp:205-230
Timestamp: 2025-04-09T17:15:24.552Z
Learning: Documentation should be added to new functions in the spider codebase, as already discussed with the user.
📚 Learning: 2025-08-03T01:49:27.764Z
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: build-tasks.yaml:21-45
Timestamp: 2025-08-03T01:49:27.764Z
Learning: In the spider project, external tools like `uv`, Python, and Task are documented as requirements in README.md and are expected to be pre-installed by users rather than bootstrapped by the build system.
Applied to files:
python/pyproject.toml
📚 Learning: 2025-08-03T01:55:13.870Z
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: python/spider/core/pyproject.toml:6-8
Timestamp: 2025-08-03T01:55:13.870Z
Learning: In the spider project, the "Private :: Do Not Upload" classifier in pyproject.toml files is used with `uv` package manager to prevent packages from being published to PyPI, and this is a valid approach in the `uv` ecosystem.
Applied to files:
python/pyproject.toml
📚 Learning: 2025-04-09T17:15:24.552Z
Learnt from: davidlion
PR: y-scope/spider#100
File: src/spider/worker/worker.cpp:205-230
Timestamp: 2025-04-09T17:15:24.552Z
Learning: Documentation should be added to new functions in the spider codebase, as already discussed with the user.
Applied to files:
python/pyproject.toml
📚 Learning: 2025-08-03T01:55:13.870Z
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: python/spider/core/pyproject.toml:6-8
Timestamp: 2025-08-03T01:55:13.870Z
Learning: The "Private :: Do Not Upload" classifier in pyproject.toml is a valid and official Trove classifier that prevents packages from being uploaded to PyPI. This is the recommended approach for internal packages in the spider project when using uv as the package manager.
Applied to files:
python/pyproject.toml
📚 Learning: 2025-08-03T01:52:26.847Z
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: python/spider/client/pyproject.toml:6-7
Timestamp: 2025-08-03T01:52:26.847Z
Learning: In the spider project, internal dependencies between packages (like spider-client depending on spider-core) are managed without version pinning since they are developed together in the same repository and versions are kept synchronized.
Applied to files:
python/pyproject.toml
📚 Learning: 2025-08-03T01:54:03.099Z
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: build-tasks.yaml:26-29
Timestamp: 2025-08-03T01:54:03.099Z
Learning: In Task build systems, glob patterns like "python/**/*" properly match files at any nested level, including package-level pyproject.toml files in subdirectories like "python/spider/client/pyproject.toml". The "**" wildcard recursively matches any number of directory levels.
Applied to files:
python/pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (3)
python/pyproject.toml (3)
12-13: Console-script path looks correct nowThe entry point points to spider.task_executor.task_executor:main, which matches the installed package namespace (not src.). This resolves the earlier ModuleNotFoundError risk.
5-5: README path verified TheREADME.mdfile exists atpython/README.md, so thereadme = "README.md"setting inpyproject.tomlis correct.
21-24: Hatchling src-layout not configured — wheel may be empty or miss packagesYou’re using a src/ layout (per repo structure and entry point), but hatchling isn’t told where to find packages. Without explicit package discovery, builds can omit python/src/spider. Add hatch build targets for wheel/sdist:
[build-system] requires = ["hatchling>=1.18.0"] build-backend = "hatchling.build" +[tool.hatch.build.targets.wheel] +# Point hatchling to the src/ package(s) +packages = ["src/spider"] + +[tool.hatch.build.targets.sdist] +# Ensure sources and metadata are included in the sdist +include = [ + "src/**", + "pyproject.toml", + "README.md", +]Also applies to: 1-11
⛔ Skipped due to learnings
Learnt from: sitaowang1998 PR: y-scope/spider#172 File: python/spider/core/pyproject.toml:6-8 Timestamp: 2025-08-03T01:55:13.870Z Learning: In the spider project, the "Private :: Do Not Upload" classifier in pyproject.toml files is used with `uv` package manager to prevent packages from being published to PyPI, and this is a valid approach in the `uv` ecosystem.Learnt from: sitaowang1998 PR: y-scope/spider#172 File: build-tasks.yaml:21-45 Timestamp: 2025-08-03T01:49:27.764Z Learning: In the spider project, external tools like `uv`, Python, and Task are documented as requirements in README.md and are expected to be pre-installed by users rather than bootstrapped by the build system.Learnt from: sitaowang1998 PR: y-scope/spider#172 File: python/spider/core/pyproject.toml:6-8 Timestamp: 2025-08-03T01:55:13.870Z Learning: The "Private :: Do Not Upload" classifier in pyproject.toml is a valid and official Trove classifier that prevents packages from being uploaded to PyPI. This is the recommended approach for internal packages in the spider project when using uv as the package manager.Learnt from: sitaowang1998 PR: y-scope/spider#172 File: build-tasks.yaml:26-29 Timestamp: 2025-08-03T01:54:03.099Z Learning: In Task build systems, glob patterns like "python/**/*" properly match files at any nested level, including package-level pyproject.toml files in subdirectories like "python/spider/client/pyproject.toml". The "**" wildcard recursively matches any number of directory levels.Learnt from: kirkrodrigues PR: y-scope/spider#1 File: .github/workflows/pr-title.yaml:11-15 Timestamp: 2024-10-23T02:23:12.161Z Learning: Ensure to carefully check YAML indentation before suggesting fixes, and avoid removing comments unless necessary.
5 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
python/spider-py/src/spider_py/core/data.py (2)
3-14: Generate IDs by default and make the alias explicit with TypeAliasProviding a default ID simplifies callers and reduces boilerplate; clarifying the alias improves readability for type-checkers.
Apply:
-from dataclasses import dataclass -from uuid import UUID +from dataclasses import dataclass, field +from typing import TypeAlias +from uuid import UUID, uuid4 @@ -DataId = UUID +DataId: TypeAlias = UUID @@ - id: DataId + id: DataId = field(default_factory=uuid4)
9-14: Consider immutability and slots for a lightweight value objectIf Data is meant to be a pure value object, making it frozen and using slots can prevent accidental mutation and cut per-instance memory.
-@dataclass +@dataclass(frozen=True, slots=True) class Data:python/spider-py/src/spider_py/core/taskgraph.py (2)
11-17: Avoid duplicate edges by using a set for dependenciesUsing a set eliminates redundant edges and guards against accidental duplicates when add_task is called repeatedly.
- self.dependencies: list[tuple[TaskId, TaskId]] = [] + self.dependencies: set[tuple[TaskId, TaskId]] = set()And update the mutation sites:
- self.dependencies.append((parent, task.task_id)) + self.dependencies.add((parent, task.task_id)) @@ - self.dependencies.append((task.task_id, child)) + self.dependencies.add((task.task_id, child))
9-17: Adjacency indices could improve lookup complexity (optional)Repeated O(E) scans in get_parents/get_children are fine for small graphs. If you expect large graphs, consider maintaining parent/child adjacency maps to get O(out-degree)/O(in-degree) lookups.
python/spider-py/src/spider_py/core/task.py (2)
12-18: Clarify indexing semantics for TaskInputOutput.positionSpecify whether position is zero-based or one-based to avoid off-by-one errors across producers/consumers.
20-27: Unions of raw bytes and IDs are ambiguous at runtimeTaskInput/TaskOutput mix raw bytes with ID references. At runtime, disambiguation requires isinstance checks and can be error-prone if types evolve. Consider tagged wrappers (e.g., dataclasses DataRef/BytesValue) for discriminated unions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
python/spider-py/src/spider_py/core/__init__.py(1 hunks)python/spider-py/src/spider_py/core/data.py(1 hunks)python/spider-py/src/spider_py/core/task.py(1 hunks)python/spider-py/src/spider_py/core/taskgraph.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T13:45:33.914Z
Learnt from: sitaowang1998
PR: y-scope/spider#180
File: python/src/core/taskgraph.py:50-56
Timestamp: 2025-08-06T13:45:33.914Z
Learning: In the TaskGraph class in python/src/core/taskgraph.py, there is a program invariant that ensures all task IDs in TaskGraph.dependencies are guaranteed to exist in TaskGraph.tasks. The add_task method enforces this by requiring parent and children tasks to already be present in the graph before adding dependencies, eliminating the need for error handling in get_parents and get_children methods.
Applied to files:
python/spider-py/src/spider_py/core/taskgraph.py
🧬 Code Graph Analysis (2)
python/spider-py/src/spider_py/core/__init__.py (3)
python/spider-py/src/spider_py/core/data.py (1)
Data(10-14)python/spider-py/src/spider_py/core/task.py (3)
Task(41-50)TaskInputOutput(13-17)TaskState(29-37)python/spider-py/src/spider_py/core/taskgraph.py (1)
TaskGraph(6-56)
python/spider-py/src/spider_py/core/taskgraph.py (1)
python/spider-py/src/spider_py/core/task.py (1)
Task(41-50)
🔇 Additional comments (2)
python/spider-py/src/spider_py/core/__init__.py (1)
3-32: LGTM: public API surface is coherent and completeRe-exports are consistent with the new modules; all accurately reflects the intended surface.
python/spider-py/src/spider_py/core/taskgraph.py (1)
42-56: Potential KeyError if invariants are violatedget_parents/get_children index into self.tasks without guarding against missing IDs. If you choose not to enforce invariants in add_task, add a safe fallback or clearer error. With the validation added above, this remains safe.
Do you want explicit error messages here (e.g., raising KeyError with context) or are the add_task guards sufficient for your use-case?
Comment on lines
+44
to
+50
| task_id: TaskId = field(default_factory=uuid4) | ||
| function_name: str = "" | ||
| state: TaskState = TaskState.Pending | ||
| timeout: float = 0 | ||
| max_retries: int = 0 | ||
| task_inputs: list[TaskInput] = field(default_factory=list) | ||
| task_outputs: list[TaskOutput] = field(default_factory=list) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate basic Task invariants (non-negative timeout and retries)
Prevent obviously invalid state early.
@dataclass
class Task:
@@
task_outputs: list[TaskOutput] = field(default_factory=list)
+
+ def __post_init__(self) -> None:
+ if self.timeout < 0:
+ raise ValueError("timeout must be non-negative")
+ if self.max_retries < 0:
+ raise ValueError("max_retries must be non-negative")🤖 Prompt for AI Agents
In python/spider-py/src/spider_py/core/task.py around lines 44 to 50, add
validation to enforce non-negative timeout and max_retries by implementing a
__post_init__ on the dataclass that checks if self.timeout < 0 or
self.max_retries < 0 and raises a ValueError with a clear message (e.g.,
"timeout must be >= 0" / "max_retries must be >= 0"); keep existing defaults and
types, and ensure tests or callers that create Task instances will see the
exception immediately for invalid values.
Comment on lines
+19
to
+40
| def add_task( | ||
| self, task: Task, parents: list[TaskId] | None = None, children: list[TaskId] | None = None | ||
| ) -> None: | ||
| """ | ||
| Adds a task to the graph. | ||
| :param task: The task to add. | ||
| :param parents: The parent ids of the task. Must be already in the task graph. | ||
| :param children: The children ids of the task. Must be already in the task graph. | ||
| """ | ||
| self.tasks[task.task_id] = task | ||
| if parents: | ||
| for parent in parents: | ||
| self.dependencies.append((parent, task.task_id)) | ||
| self.output_tasks.discard(parent) | ||
| else: | ||
| self.input_tasks.add(task.task_id) | ||
| if children: | ||
| for child in children: | ||
| self.dependencies.append((task.task_id, child)) | ||
| self.input_tasks.discard(child) | ||
| else: | ||
| self.output_tasks.add(task.task_id) |
Contributor
There was a problem hiding this comment.
Enforce graph invariants and prevent silent overwrites
Currently, add_task does not validate that parents/children exist nor that task_id is unique, despite the docstring implying this precondition. This can introduce dangling edges and allow accidental overwrites.
Apply:
def add_task(
self, task: Task, parents: list[TaskId] | None = None, children: list[TaskId] | None = None
) -> None:
@@
- self.tasks[task.task_id] = task
+ # Validate constraints before mutating state
+ if task.task_id in self.tasks:
+ raise ValueError(f"Task with id {task.task_id} already exists in the graph")
+ if parents:
+ for parent in parents:
+ if parent not in self.tasks:
+ raise ValueError(f"Parent task id {parent} is not present in the graph")
+ if children:
+ for child in children:
+ if child not in self.tasks:
+ raise ValueError(f"Child task id {child} is not present in the graph")
+
+ self.tasks[task.task_id] = task
if parents:
for parent in parents:
self.dependencies.append((parent, task.task_id))
self.output_tasks.discard(parent)
else:
self.input_tasks.add(task.task_id)
if children:
for child in children:
self.dependencies.append((task.task_id, child))
self.input_tasks.discard(child)
else:
self.output_tasks.add(task.task_id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def add_task( | |
| self, task: Task, parents: list[TaskId] | None = None, children: list[TaskId] | None = None | |
| ) -> None: | |
| """ | |
| Adds a task to the graph. | |
| :param task: The task to add. | |
| :param parents: The parent ids of the task. Must be already in the task graph. | |
| :param children: The children ids of the task. Must be already in the task graph. | |
| """ | |
| self.tasks[task.task_id] = task | |
| if parents: | |
| for parent in parents: | |
| self.dependencies.append((parent, task.task_id)) | |
| self.output_tasks.discard(parent) | |
| else: | |
| self.input_tasks.add(task.task_id) | |
| if children: | |
| for child in children: | |
| self.dependencies.append((task.task_id, child)) | |
| self.input_tasks.discard(child) | |
| else: | |
| self.output_tasks.add(task.task_id) | |
| def add_task( | |
| self, task: Task, parents: list[TaskId] | None = None, children: list[TaskId] | None = None | |
| ) -> None: | |
| """ | |
| Adds a task to the graph. | |
| :param task: The task to add. | |
| :param parents: The parent ids of the task. Must be already in the task graph. | |
| :param children: The children ids of the task. Must be already in the task graph. | |
| """ | |
| # Validate constraints before mutating state | |
| if task.task_id in self.tasks: | |
| raise ValueError(f"Task with id {task.task_id} already exists in the graph") | |
| if parents: | |
| for parent in parents: | |
| if parent not in self.tasks: | |
| raise ValueError(f"Parent task id {parent} is not present in the graph") | |
| if children: | |
| for child in children: | |
| if child not in self.tasks: | |
| raise ValueError(f"Child task id {child} is not present in the graph") | |
| self.tasks[task.task_id] = task | |
| if parents: | |
| for parent in parents: | |
| self.dependencies.append((parent, task.task_id)) | |
| self.output_tasks.discard(parent) | |
| else: | |
| self.input_tasks.add(task.task_id) | |
| if children: | |
| for child in children: | |
| self.dependencies.append((task.task_id, child)) | |
| self.input_tasks.discard(child) | |
| else: | |
| self.output_tasks.add(task.task_id) |
🤖 Prompt for AI Agents
In python/spider-py/src/spider_py/core/taskgraph.py around lines 19 to 40, add
strict validation before mutating state: first check if task.task_id already
exists and raise a ValueError to prevent silent overwrites; then if parents or
children are provided, verify every referenced id exists in self.tasks and raise
a KeyError (or ValueError) for any missing id so no dangling edges are created;
perform these validations before adding task to self.tasks and appending
dependencies so the graph update is atomic on success only; after validations,
add the task and then update self.dependencies, self.input_tasks and
self.output_tasks exactly as currently done.
LinZhihao-723
requested changes
Aug 14, 2025
LinZhihao-723
left a comment
LinZhihao-723
left a comment
Member
There was a problem hiding this comment.
Before we proceed: shall we add __init__ method for Data and Task?
LinZhihao-723
requested changes
Aug 14, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
python/spider-py/src/spider_py/core/task_graph.py (2)
23-25: Consider indexing for performance and duplicate-edge control.Scanning
dependencieson every query is O(E). For medium/large graphs, maintain adjacency maps to makeget_parents/get_childrenO(out-degree/in-degree):
- parent_map: dict[TaskId, set[TaskId]] for children
- child_map: dict[TaskId, set[TaskId]] for parents
- Optionally, keep an internal set
_dependency_setto dedupe edges in O(1).This keeps the public API intact while improving query performance and preventing duplicate edges.
36-36: Clarify semantics for re-adding an existing task ID.
self.tasks[task.task_id] = tasksilently overwrites an existing task if the same ID is used again. Is that intended? If not, guard with a check and raiseValueError. If yes, document thatadd_taskcan be used to upsert task metadata and add edges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
python/spider-py/src/spider_py/core/__init__.py(1 hunks)python/spider-py/src/spider_py/core/task.py(1 hunks)python/spider-py/src/spider_py/core/task_graph.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- python/spider-py/src/spider_py/core/task.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T13:45:33.914Z
Learnt from: sitaowang1998
PR: y-scope/spider#180
File: python/src/core/taskgraph.py:50-56
Timestamp: 2025-08-06T13:45:33.914Z
Learning: In the TaskGraph class in python/src/core/taskgraph.py, there is a program invariant that ensures all task IDs in TaskGraph.dependencies are guaranteed to exist in TaskGraph.tasks. The add_task method enforces this by requiring parent and children tasks to already be present in the graph before adding dependencies, eliminating the need for error handling in get_parents and get_children methods.
Applied to files:
python/spider-py/src/spider_py/core/task_graph.py
🧬 Code Graph Analysis (2)
python/spider-py/src/spider_py/core/task_graph.py (1)
python/spider-py/src/spider_py/core/task.py (1)
Task(41-50)
python/spider-py/src/spider_py/core/__init__.py (3)
python/spider-py/src/spider_py/core/data.py (1)
Data(10-14)python/spider-py/src/spider_py/core/task.py (3)
Task(41-50)TaskInputOutput(13-17)TaskState(29-37)python/spider-py/src/spider_py/core/task_graph.py (1)
TaskGraph(6-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (5)
python/spider-py/src/spider_py/core/task_graph.py (4)
50-56: Guard unknown task_id and improve docstring.If
task_idisn’t present, this comprehension will KeyError viaself.tasks[parent]. Consider failing fast with a clear message and make the param doc explicit.def get_parents(self, task_id: TaskId) -> list[Task]: - """ - :param task_id: - :return: Parent tasks of the task identified by `task_id`. - """ - return [self.tasks[parent] for (parent, child) in self.dependencies if child == task_id] + """ + :param task_id: ID of the task whose parents are requested. Must exist in the graph. + :return: Parent tasks of the task identified by `task_id`. + """ + if task_id not in self.tasks: + raise ValueError(f"Unknown task_id: {task_id}") + return [self.tasks[parent] for (parent, child) in self.dependencies if child == task_id]⛔ Skipped due to learnings
Learnt from: sitaowang1998 PR: y-scope/spider#180 File: python/src/core/taskgraph.py:50-56 Timestamp: 2025-08-06T13:45:33.914Z Learning: In the TaskGraph class in python/src/core/taskgraph.py, there is a program invariant that ensures all task IDs in TaskGraph.dependencies are guaranteed to exist in TaskGraph.tasks. The add_task method enforces this by requiring parent and children tasks to already be present in the graph before adding dependencies, eliminating the need for error handling in get_parents and get_children methods.
57-62: Mirror the guard and doc improvements in get_children.def get_children(self, task_id: TaskId) -> list[Task]: - """ - :param task_id: - :return: Child tasks of the task identified by `task_id`. - """ - return [self.tasks[child] for (parent, child) in self.dependencies if parent == task_id] + """ + :param task_id: ID of the task whose children are requested. Must exist in the graph. + :return: Child tasks of the task identified by `task_id`. + """ + if task_id not in self.tasks: + raise ValueError(f"Unknown task_id: {task_id}") + return [self.tasks[child] for (parent, child) in self.dependencies if parent == task_id]⛔ Skipped due to learnings
Learnt from: sitaowang1998 PR: y-scope/spider#180 File: python/src/core/taskgraph.py:50-56 Timestamp: 2025-08-06T13:45:33.914Z Learning: In the TaskGraph class in python/src/core/taskgraph.py, there is a program invariant that ensures all task IDs in TaskGraph.dependencies are guaranteed to exist in TaskGraph.tasks. The add_task method enforces this by requiring parent and children tasks to already be present in the graph before adding dependencies, eliminating the need for error handling in get_parents and get_children methods.
36-48: Enforce graph integrity: validate existence, prevent self-loops and cycles, and dedupe edges.
- If a parent/child ID isn’t in
self.tasks,get_parents/get_childrenwill later raise a KeyError viaself.tasks[...].- Self-dependencies are allowed today.
- Cycles are possible (contradicts DAG claim).
- Duplicate edges can accumulate.
Apply the following minimal fix to validate membership, avoid self-loops, detect cycles, and dedupe duplicates from the provided lists:
@@ - self.tasks[task.task_id] = task - if parents: - for parent in parents: - self.dependencies.append((parent, task.task_id)) - self.output_tasks.discard(parent) + self.tasks[task.task_id] = task + + # Validate and add parent edges + if parents: + for parent in set(parents): + if parent == task.task_id: + raise ValueError("A task cannot depend on itself") + if parent not in self.tasks: + raise ValueError(f"Parent task {parent} is not in the graph") + # Prevent cycles: adding parent->task creates a cycle if task can already reach parent + if self._has_path(task.task_id, parent): + raise ValueError(f"Adding edge {parent} -> {task.task_id} would create a cycle") + if (parent, task.task_id) not in self.dependencies: + self.dependencies.append((parent, task.task_id)) + self.output_tasks.discard(parent) else: self.input_tasks.add(task.task_id) - if children: - for child in children: - self.dependencies.append((task.task_id, child)) - self.input_tasks.discard(child) + # Validate and add child edges + if children: + for child in set(children): + if child == task.task_id: + raise ValueError("A task cannot depend on itself") + if child not in self.tasks: + raise ValueError(f"Child task {child} is not in the graph") + # Prevent cycles: adding task->child creates a cycle if child can already reach task + if self._has_path(child, task.task_id): + raise ValueError(f"Adding edge {task.task_id} -> {child} would create a cycle") + if (task.task_id, child) not in self.dependencies: + self.dependencies.append((task.task_id, child)) + self.input_tasks.discard(child) else: self.output_tasks.add(task.task_id)Add this helper inside the class to support the cycle checks:
def _has_path(self, src: TaskId, dst: TaskId) -> bool: """Returns True if there is a path src -> ... -> dst using current dependencies.""" if src == dst: return True # Build adjacency on the fly adj: dict[TaskId, set[TaskId]] = {} for u, v in self.dependencies: adj.setdefault(u, set()).add(v) # DFS seen: set[TaskId] = set() stack = [src] while stack: u = stack.pop() if u in seen: continue seen.add(u) for v in adj.get(u, ()): if v == dst: return True if v not in seen: stack.append(v) return FalseThis aligns with the invariant noted in earlier discussions that all dependency IDs must exist in
tasks, and it ensures the DAG guarantee holds.⛔ Skipped due to learnings
Learnt from: sitaowang1998 PR: y-scope/spider#180 File: python/src/core/taskgraph.py:50-56 Timestamp: 2025-08-06T13:45:33.914Z Learning: In the TaskGraph class in python/src/core/taskgraph.py, there is a program invariant that ensures all task IDs in TaskGraph.dependencies are guaranteed to exist in TaskGraph.tasks. The add_task method enforces this by requiring parent and children tasks to already be present in the graph before adding dependencies, eliminating the need for error handling in get_parents and get_children methods.
6-18: Enforce DAG invariants in TaskGraph.add_task — reject self‑loops/cycles and validate parents/childrenI inspected python/spider-py/src/spider_py/core/task_graph.py; add_task (def at line 27) currently sets self.tasks and then appends dependencies without checking existence, self‑loops or cycles — the docstring's "DAG" guarantee is not enforced.
Location to change
- python/spider-py/src/spider_py/core/task_graph.py — def add_task (starts at line 27)
Problems found
- Parent/child IDs are not validated against self.tasks (docstring says they "must be already in the task graph").
- Self‑loops are allowed (parent == task.task_id or child == task.task_id).
- Cycles are not prevented (no reachability check before adding an edge).
Minimal required fixes
- Validate parents/children exist before adding edges; raise a clear error if missing.
- Reject explicit self‑loops (raise ValueError).
- Prevent cycles: before adding an edge u->v, check whether v already reaches u in the current graph (if so, reject). Implement a helper like _path_exists(src, dst) using DFS/BFS on adjacency built from self.dependencies.
- Only update input_tasks/output_tasks and self.dependencies after all validations pass.
Example (concise) check to add before appending an edge:
- if parent == task.task_id: raise ValueError("self-loop not allowed")
- if parent not in self.tasks: raise KeyError(f"parent {parent} not found")
- if self._path_exists(task.task_id, parent): raise ValueError("would create cycle")
Please apply these fixes and add tests that assert self‑loops and cycle insertions are rejected.
⛔ Skipped due to learnings
Learnt from: sitaowang1998 PR: y-scope/spider#180 File: python/src/core/taskgraph.py:50-56 Timestamp: 2025-08-06T13:45:33.914Z Learning: In the TaskGraph class in python/src/core/taskgraph.py, there is a program invariant that ensures all task IDs in TaskGraph.dependencies are guaranteed to exist in TaskGraph.tasks. The add_task method enforces this by requiring parent and children tasks to already be present in the graph before adding dependencies, eliminating the need for error handling in get_parents and get_children methods.python/spider-py/src/spider_py/core/__init__.py (1)
3-32: Centralised re-exports look good.Public surface is coherent and imports align with all. No concerns.
LinZhihao-723
requested changes
Aug 15, 2025
LinZhihao-723
left a comment
LinZhihao-723
left a comment
Member
There was a problem hiding this comment.
for the PR title, how about:
feat(spider-py): Add `Data`, `Task`, and `TaskGraph` to core.
LinZhihao-723
approved these changes
Aug 15, 2025
sitaowang1998
changed the title
Data, Task, and TaskGraph to core.
3 tasks
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
2 participants
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
Note
This PR depends on #179.
This PR adds core Python Data, Task and TaskGraph classes for later use of client and task executor.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit