feat(tdl): Add end-to-end TDL package execution flow (resolves #302): - #317
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughThis PR implements the TDL (Task Definition Language) package framework, enabling users to write custom task functions in Rust, compile them as shared libraries, and load them at runtime via C-FFI. It introduces ChangesTDL Package Framework Implementation
Sequence DiagramsequenceDiagram
participant Storage
participant Executor as Task Executor
participant Manager as Package Manager
participant Loader as Dynamic Loader
participant Package as TDL Package (cdylib)
Storage->>Executor: task_name + context + inputs (bytes)
Executor->>Manager: load_package(path)
Manager->>Loader: dlopen(cdylib_path)
Loader->>Package: __spider_tdl_package_get_version()
Package-->>Loader: Version struct
Loader-->>Manager: Validate version compatibility
Loader->>Package: __spider_tdl_package_get_name()
Package-->>Loader: Package name bytes
Loader-->>Manager: Register package in HashMap
Manager-->>Executor: Package loaded
Executor->>Manager: get_package(name)
Manager-->>Executor: TdlPackage ref
Executor->>Package: __spider_tdl_package_execute(task_name, ctx_bytes, inputs_bytes)
Package->>Package: Deserialize context + inputs
Package->>Package: Dispatch to task handler
Package->>Package: Execute task logic
Package->>Package: Serialize output or error
Package-->>Executor: TaskExecutionResult (output_bytes or error_bytes)
Executor->>Executor: Deserialize output/error
Executor-->>Storage: output_bytes or ExecutorError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes This PR introduces foundational FFI infrastructure ( Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/spider-tdl-derive/src/task_macro.rs (1)
158-162: ⚡ Quick winHide the generated params type from public docs.
Now that this struct can become
pub, public tasks will expose__<task>_paramsas synthetic API surface. Adding#[doc(hidden)]keeps the privacy fix without advertising macro internals.Proposed change
#[allow(non_camel_case_types)] + #[doc(hidden)] #[derive(::serde::Deserialize)] `#vis` struct `#params_struct_name` { #(`#param_fields`,)* }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/spider-tdl-derive/src/task_macro.rs` around lines 158 - 162, The generated params struct (the symbol represented by `#params_struct_name`, e.g. the synthetic __<task>_params type) is being emitted without hiding it from public docs; add the #[doc(hidden)] attribute to the struct declaration in task_macro.rs (the block that creates "struct `#params_struct_name` { #(`#param_fields`,)* }") so that when the struct is generated as `pub` it is excluded from API documentation while preserving the privacy fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/spider-tdl/src/register.rs`:
- Around line 91-128: The extern "C" function __spider_tdl_package_execute must
not allow Rust panics to unwind across the C ABI; wrap the call to
handler.execute_raw (the place where user task code runs) in
std::panic::catch_unwind (use std::panic::AssertUnwindSafe if necessary) and
convert any panic into a serialized TdlError payload (e.g. a TdlError variant
like Panic or a generic DeserializationError) using serialize_error_payload,
then return it via $crate::ffi::TaskExecutionResult::from_error; keep the
existing branches for Ok(output_bytes) and Err(error_bytes) but add a panic
branch that serializes the panic information and returns it instead of letting
the panic propagate.
In `@taskfiles/test.yaml`:
- Line 217: SPIDER_TDL_PACKAGE_COMPLEX currently hardcodes a Linux .so suffix;
update its value to pick the correct Rust cdylib extension based on the go-task
{{OS}} template (e.g. use a template conditional on .OS: return ".dylib" when
.OS == "darwin" and ".so" for Unix/Linux, optionally ".dll" for Windows). Modify
the SPIDER_TDL_PACKAGE_COMPLEX assignment that uses {{.G_RUST_BUILD_DIR}} so it
appends the chosen extension via the conditional (refer to the
SPIDER_TDL_PACKAGE_COMPLEX key and G_RUST_BUILD_DIR symbol in the file).
---
Nitpick comments:
In `@components/spider-tdl-derive/src/task_macro.rs`:
- Around line 158-162: The generated params struct (the symbol represented by
`#params_struct_name`, e.g. the synthetic __<task>_params type) is being emitted
without hiding it from public docs; add the #[doc(hidden)] attribute to the
struct declaration in task_macro.rs (the block that creates "struct
`#params_struct_name` { #(`#param_fields`,)* }") so that when the struct is
generated as `pub` it is excluded from API documentation while preserving the
privacy fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 96a04a54-318b-4b4d-8fe3-719e9dafb5f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlcomponents/spider-task-executor/Cargo.tomlcomponents/spider-task-executor/src/error.rscomponents/spider-task-executor/src/lib.rscomponents/spider-task-executor/src/manager.rscomponents/spider-tdl-derive/src/task_macro.rscomponents/spider-tdl/src/lib.rscomponents/spider-tdl/src/register.rscomponents/spider-tdl/src/version.rsexamples/huntsman/complex/tasks/Cargo.tomlexamples/huntsman/complex/tasks/src/lib.rsexamples/huntsman/complex/types/Cargo.tomlexamples/huntsman/complex/types/src/lib.rstaskfiles/test.yamltests/huntsman/tdl-integration/Cargo.tomltests/huntsman/tdl-integration/src/lib.rstests/huntsman/tdl-integration/tests/complex.rs
sitaowang1998
left a comment
sitaowang1998
left a comment
There was a problem hiding this comment.
The overall design look good to me. Just a few implementation details.
| //! 2. A private params struct holding the non-context parameters, with | ||
| //! 2. A params struct holding the non-context parameters (sharing the function's visibility, so it | ||
| //! never leaks through the public `Task::Params` associated type), with | ||
| //! `#[derive(serde::Deserialize)]` so the runtime can rebuild it from wire bytes. |
There was a problem hiding this comment.
Can we make the phrase inside the parentheses a separate sentence?
| /// * `s` is an empty string. | ||
| /// * `s` contains a non-ASCII-digit byte. | ||
| /// * The parsed value would overflow `u32`. | ||
| const fn const_parse_u32(s: &str) -> u32 { |
There was a problem hiding this comment.
Ok, we can use const_str for comparison and number parsing. The uniqueness check can't be rewritten using any of these libraries.
2 participants
register_tdl_packagemacro for TDL package declaration.spider-task-executorcrate and TDL package manager for loading packages at runtime.Description
This is the last PR to resolve #302.
This PR adds end-to-end TDL package execution flow:
Task registration
We add
register_tdl_packagemacro inspider-tdlfor TDL package declaration. The macro takes a package name and a list of tasks, and generates C-ABI symbols for accessing the underlying tasks. The TDL package compiles into a cdylib, while it is expected that these symbols are global to each package (so thatregister_tdl_packageshould be unique in a crate).The package identifies TDL packages uniquely by the package name. The macro detects duplicate names at compile time.
spider-task-executorThis is a new crate for the executor implementation. As the initial commit to this crate, we introduce the TDL package manager that wraps the cdylib and exposes safe Rust APIs to execute the tasks. Read the in-file docstrings for details.
Example: complex
This PR introduces an example TDL package,
complex, which contains complex number type declaration and vector operations, including add, sub, dot-product, and cross-product.An integration test is added to test the example, using the TDL package manager to load the compiled library. By doing so, we have built an end-to-end flow for:
TDL task declaration -> TDL package creation -> TDL task execution inside the Spider task executor
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Release Notes
New Features
Tests