feat(spider-py): Add boilerplate for spider-py; Add build and lint tasks for spider-py. - #179
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/src/spider_py/core/task.py (1)
4-5: Define a minimal Task contract (or add a pass) to guide future implementationDocstring-only classes are valid, but a tiny contract improves clarity and testability. Two options:
- Short-term: keep it a stub but add a pass to appease stricter linters.
- Next step: evolve into a dataclass/Protocol with minimal identifiers and payload semantics.
Apply this small stub improvement now:
class Task: - """Represents a task in Spider.""" + """Represents a task in Spider.""" + # TODO: Define minimal fields and lifecycle semantics (id, payload, retries, etc.). + passIf you prefer jumping straight to a contract, I can propose a dataclass skeleton with id/payload/metadata. Want me to draft it?
python/src/spider_py/task_executor/task_executor.py (1)
4-9: Return an exit code and use SystemExit for CLI behaviourReturning an int and raising SystemExit makes the entry point composable, testable, and CLI-friendly.
-def main() -> None: - """Main function to execute the task.""" +def main() -> int: + """CLI entry point to execute a Spider Python task.""" + # TODO: Implement task execution logic. + return 0 @@ -if __name__ == "__main__": - main() +if __name__ == "__main__": + raise SystemExit(main())Follow-ups (optional): wire argparse, logging, and structured error handling. I can draft a minimal argparse scaffold if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
python/pyproject.toml(1 hunks)python/src/spider_py/__init__.py(1 hunks)python/src/spider_py/client/__init__.py(1 hunks)python/src/spider_py/core/__init__.py(1 hunks)python/src/spider_py/core/task.py(1 hunks)python/src/spider_py/task_executor/__init__.py(1 hunks)python/src/spider_py/task_executor/task_executor.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- python/src/spider_py/init.py
- python/src/spider_py/task_executor/init.py
- python/src/spider_py/client/init.py
- python/src/spider_py/core/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (1)
python/src/spider_py/core/task.py (1)
1-5: Minimal scaffold LGTMGood foundational placeholder for the core entity.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Overall lgtm. Left some structural comments.
| py-build: | ||
| cmds: | ||
| - "uv build --directory {{.G_SRC_PYTHON_DIR}} -o {{.G_BUILD_PYTHON_DIR}}" |
There was a problem hiding this comment.
Two comments for this task:
- Can we rename the task to be
spider-py? We should dropbuildas it's already underbuildtask namespace. - Optional: Can we set the build directory to be
$ROOT/python/spider-py/build? (assuming u apply the comment for addingspider-pyunderpython) In this way, we can usetask build:spider-pyinside$ROOT/python/spider-py/build, treating it as a stand-alone project with the build results directly located inside the project.
There was a problem hiding this comment.
- Task renaming done.
- I think we should use only one build directory for all components, mirroring the behavior in
clp.
There was a problem hiding this comment.
Can we draft a simple README to include how to build and lint this project?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lint-tasks.yaml (1)
142-164: Implementation matches the suggested approach from past reviews.The spider-py lint task implementation follows the pattern suggested in previous review comments, with proper flag handling and uv integration.
🧹 Nitpick comments (3)
python/spider-py/src/spider_py/task_executor/task_executor.py (1)
4-6: Implement the main function body.The main function is currently empty and needs implementation to fulfill its documented purpose of executing Spider tasks.
Would you like me to generate a basic implementation structure for the task executor's main function?
python/spider-py/src/spider_py/core/task.py (1)
4-5: Consider adding basic task properties and methods.The Task class is currently a skeleton with no attributes or methods. For a meaningful task execution framework, consider adding basic properties like task ID, status, and execution methods.
Add basic task structure:
class Task: """Represents a task in Spider.""" + + def __init__(self, task_id: str) -> None: + """Initialize a task with the given ID.""" + self.task_id = task_id + self.status = "pending" + + def execute(self) -> None: + """Execute the task.""" + raise NotImplementedError("Subclasses must implement execute method")python/spider-py/pyproject.toml (1)
40-41: Consider being more selective with lint rules.Using
select = ["ALL"]enables every available lint rule, which can be overly strict for a new project. The extensive ignore list (Lines 42-60) suggests many rules aren't applicable.Consider starting with a curated set of essential rules:
[tool.ruff.lint] -select = ["ALL"] -extend-select = ["PT"] +select = [ + "E", # pycodestyle errors + "W", # pycodestyle warnings + "F", # pyflakes + "I", # isort + "N", # pep8-naming + "UP", # pyupgrade + "B", # flake8-bugbear + "C4", # flake8-comprehensions + "PT", # flake8-pytest-style +]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
python/spider-py/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
build-tasks.yaml(1 hunks)lint-tasks.yaml(2 hunks)python/spider-py/.python-version(1 hunks)python/spider-py/pyproject.toml(1 hunks)python/spider-py/src/spider_py/__init__.py(1 hunks)python/spider-py/src/spider_py/client/__init__.py(1 hunks)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/task_executor/__init__.py(1 hunks)python/spider-py/src/spider_py/task_executor/task_executor.py(1 hunks)taskfile.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- python/spider-py/src/spider_py/client/init.py
- python/spider-py/src/spider_py/task_executor/init.py
- python/spider-py/src/spider_py/core/init.py
- python/spider-py/.python-version
- python/spider-py/src/spider_py/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- build-tasks.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/spider-py/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-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (5)
taskfile.yaml (1)
23-24: LGTM! Clean variable definitions for Python component.The new variables follow the established naming convention and correctly define paths for the Python source and build directories.
python/spider-py/pyproject.toml (3)
12-13: LGTM! Correct console script configuration.The entry point correctly references the main function in the task executor module that's defined in the corresponding Python file.
5-5: Verified — README.md exists at python/spider-py/README.mdThe pyproject readme reference is correct; no changes required.
- python/spider-py/README.md
7-10: Dependency versions verified — no update requiredChecked PyPI and public advisories (as of 2025-08-13): the pinned versions are the latest stable releases and I found no public CVEs specific to these package versions. Note: the underlying msgpack runtime has had vulnerabilities in the past — if you deserialize untrusted MessagePack payloads, review and keep the runtime (e.g., python-msgpack) up-to-date and follow hardening guidance.
- File needing attention (verified):
- python/spider-py/pyproject.toml — lines 7–10
dependencies = [ "mariadb>=1.1.13", "msgpack-types>=0.5.0", ]lint-tasks.yaml (1)
118-118: LGTM! Proper integration with existing lint workflows.The spider-py tasks are correctly integrated into the existing py-check and py-fix workflows, maintaining consistency with the project's lint structure.
Also applies to: 125-125
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
python/spider-py/README.md (5)
6-6: Prefer “Building/Packaging” over “Building/Packing”.Minor wording improvement for clarity and conventional terminology.
-## Building/Packing +## Building/Packaging
17-17: Align heading with the actual build outputs (wheel + sdist).The command produces both a wheel and sdist; reflect that in the subheading for consistency with the explanation below.
-### Build Commands - -* Build a Python wheel: +### Build Commands + +* Build the package (wheel + sdist):Also applies to: 23-24
26-33: Briefly mention which linters run and what “fix” does.Add a one-liner to set expectations (ruff + mypy; “fix” applies ruff’s autofixes).
## Linting -To run all linting checks: +The following tasks run static checks using ruff and mypy. The “fix” variant applies ruff’s automatic fixes where possible. + +To run all linting checks: ```shell task lint:spider-py-check-To run all linting checks AND automatically fix any fixable issues:
+To run all linting checks and automatically apply available fixes:task lint:spider-py-fixAlso applies to: 36-38 --- `34-34`: **Tone/style nit: avoid all-caps “AND” and tighten phrasing.** ```diff -To run all linting checks AND automatically fix any fixable issues: +To run all linting checks and automatically apply available fixes:
1-5: Consider adding a short “Usage” note to clarify import name vs. distribution name.Given the project name uses a hyphen (spider-py) while the import is typically underscored (e.g., spider_py), a brief note or snippet helps avoid confusion.
If the import package is spider_py and there’s a console entry point, I can draft a concise “Usage” section (import example and CLI invocation). Please confirm the import name and the console script name you want documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
python/spider-py/README.md(1 hunks)python/spider-py/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- python/spider-py/pyproject.toml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/spider-py/README.md
📚 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/spider-py/README.md
🪛 LanguageTool
python/spider-py/README.md
[style] ~34-~34: Consider using a different verb for a more formal wording.
Context: ...un all linting checks AND automatically fix any fixable issues: ```shell task lint...
(FIX_RESOLVE)
⏰ 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-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
| * [Task] >= 3.40.0 | ||
| * [uv] >= 0.7.0 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Python to the prerequisites to align with project standards.
Per the project’s convention, Python should be documented alongside Task and uv. The repo pins Python to 3.13; make that explicit here.
### Requirements
* [Task] >= 3.40.0
* [uv] >= 0.7.0
+* Python 3.13 (see .python-version)📝 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.
| * [Task] >= 3.40.0 | |
| * [uv] >= 0.7.0 | |
| * [Task] >= 3.40.0 | |
| * [uv] >= 0.7.0 | |
| * Python 3.13 (see .python-version) |
🤖 Prompt for AI Agents
In python/spider-py/README.md around lines 12 to 14, the prerequisites list
includes Task and uv but omits Python; add an explicit Python entry pinned to
the repo version (Python >= 3.13) alongside the existing entries so the
prerequisites read include Python >= 3.13, Task >= 3.40.0, and uv >= 0.7.0.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(spider-py): Add boilerplate for spider-py; Add build and lint tasks for spider-py.
LinZhihao-723
changed the title
2 participants
Description
Note
This PR depends on #178.
This PR:
pythondirectory.pyproject.toml.pyproject.toml.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Chores
Documentation