feat(spider-core): Add support for serializing/deserializing a task graph. #281
Conversation
WalkthroughAdds JSON/MessagePack (de)serialization and schema-version compatibility checks to TaskGraph, introduces a public TaskDescriptor type and a TaskGraph insert_task API that accepts it, derives serde traits for related types, updates tests for round-trip I/O, and adds three new Cargo dependencies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@components/spider-core/Cargo.toml`:
- Around line 12-14: Remove the unused once_cell dependency declaration from
Cargo.toml (delete the once_cell = "1.21.3" entry) because the codebase uses
std::sync::LazyLock instead; then run cargo check/build to ensure no remaining
references to once_cell exist and keep the semver dependency as-is for version
validation.
In `@components/spider-core/src/task/task_graph.rs`:
- Around line 221-225: The doc comment for the field describing positional
inputs refers to `positional_inputs` but the actual field is named `inputs`;
update the documentation to use the correct field name `inputs` (e.g., change
"The vector length must match the length of `positional_inputs`." to "The vector
length must match the length of `inputs`.") so the comment around the field (in
task_graph.rs) accurately references the `inputs` field and retains the existing
explanation about Some(sources)/None semantics and length requirement.
🧹 Nitpick comments (2)
components/spider-core/src/task/task_graph.rs (2)
501-503: Preallocateserialized_tasks.This avoids reallocation churn for larger graphs.
♻️ Suggested tweak
- let mut serialized_tasks = Vec::new(); + let mut serialized_tasks = Vec::with_capacity(self.tasks.len());
946-948: Drop the debug print to keep tests quiet.This looks like leftover debugging noise.
🧹 Suggested cleanup
- let json_serialized = serde_json::to_string_pretty(&graph).expect("shouldn't fail"); - println!("{json_serialized}");
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@components/spider-core/Cargo.toml`:
- Line 12: Remove the unused once_cell dependency from the crate by deleting the
once_cell = "1.21.3" entry in Cargo.toml; ensure no code references once_cell
(the code uses std::sync::LazyLock) and run cargo check to verify compilation
still succeeds after removing the once_cell dependency declaration.
In `@components/spider-core/src/task/task_graph.rs`:
- Around line 200-228: The documentation for TaskDescriptor's input_sources
incorrectly references `positional_inputs`; update the comment to reference the
actual field name `inputs` instead (or use `inputs` consistently) so the
sentence reads that the vector length must match the length of `inputs`; ensure
this change is applied to the doc comment near the `input_sources` field in the
`TaskDescriptor` struct.
🧹 Nitpick comments (1)
components/spider-core/src/task/task_graph.rs (1)
945-946: Remove debugprintln!statement from test.This appears to be a debugging artifact. Consider removing it or, if the output is valuable, using
eprintln!within a conditional or a separate debug test.🧹 Suggested fix
- let json_serialized = serde_json::to_string_pretty(&graph).expect("shouldn't fail"); - println!("{json_serialized}"); -
| let serializable = SerializableTaskGraph::deserialize(deserializer)?; | ||
| let schema_version = Version::parse(&serializable.schema_version).map_err(|error| { | ||
| serde::de::Error::custom(format!( | ||
| "invalid schema version string '{}': {}", | ||
| serializable.schema_version, error | ||
| )) | ||
| })?; | ||
|
|
||
| if !TASK_GRAPH_SCHEMA_COMPATIBLE_VERSION_REQUIREMENT.matches(&schema_version) { | ||
| return Err(serde::de::Error::custom(format!( | ||
| "incompatible task graph schema version: found {}, compatible requirements: {}", | ||
| serializable.schema_version, TASK_GRAPH_SCHEMA_COMPATIBLE_VERSION | ||
| ))); | ||
| } |
There was a problem hiding this comment.
If we have mismatched schema version and also a change in the format, most likely the SerializableTaskGraph::deserialize will fail and user will get an error that is not incompatible schema version.
There was a problem hiding this comment.
Right. But as the motivation explained in the PR description, I think this version validation is still necessary. It is possible that our version "0.1.0" is a subset of "0.2.0", and if so, this check becomes necessary because:
- We don't reject unknown fields (but technically we could do it. But without enforcing unknown fields, users can add their own metadata to a task descriptor and it won't affect our serde).
- If a graph of "0.2.0" only uses features in "0.1.0" (for example, we add channel support in 0.2.0 but it's not used, the task graph will still be serializable in 0.1.0), it should still be invalidated by the deserializer.
To formally support multiple versions when we have different variants of schemas, we can make the actual task graph representations an enum. In that case, all possible schemas will be serializable, and this version validation becomes much more reliable. But I'm not sure if that's what we will do eventually... maintaining all supported schemas in a large enum sounds insane. We can have a selective set of it)
There was a problem hiding this comment.
What I meant is not about ditching version check. The problem is if we have a breaking change from version 0.1.0 to0.2.0, and user sends a 0.1.0 serialized task graph to 0.2.0 backend. The API server should report a version mismatch, but instead if fails to deserialize the task graph and report a deserialize error.
To make the versioning work, usually the first step is to not deserialize the whole message, but just check the version. This is a bit more verbose in implementation, especially with json, but it is more reliable.
There was a problem hiding this comment.
Yeah, I understand. I do think it's worth fixing.
I've rewritten the serde in a more low-level way to ensure this behavior. It's worth doing since this also allows streaming serialization for task descriptors.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/spider-core/tests/test_task_graph_serde.rs`:
- Around line 70-83: Update the test_missing_fields test to stop asserting on
the error string and instead pattern-match the returned error from
TaskGraph::from_json; specifically, in the Err arm match
Error::SerdeJsonError(e) and call e.is_missing_field()
(serde_json::Error::is_missing_field) to verify the missing-field case, and for
any other Err variant panic with "unexpected error variant"; leave the other
test (lines ~97–104) unchanged or consider adding a custom error variant later
since de::Error::custom() cannot be checked via is_missing_field().
🧹 Nitpick comments (1)
components/spider-core/tests/test_task_graph_serde.rs (1)
108-476: Consider moving the large JSON fixture to a dedicated test fixture file.It will be easier to edit and review without scrolling past hundreds of lines of inline JSON.
| #[test] | ||
| fn test_missing_fields() { | ||
| let expected_fields = vec!["schema_version", "tasks"]; | ||
| for field in expected_fields { | ||
| let mut task_graph: serde_json::Value = serde_json::from_str(TASK_GRAPH_IN_JSON).unwrap(); | ||
| task_graph | ||
| .as_object_mut() | ||
| .expect("task graph should be an object") | ||
| .remove(field); | ||
| match TaskGraph::from_json(&task_graph.to_string()) { | ||
| Ok(_) => panic!("deserialization should fail"), | ||
| Err(err) => assert!(err.to_string().contains("missing field")), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the TaskGraph implementation and from_json method
cd components/spider-core || exit
find . -name "*.rs" -type f | head -20Repository: y-scope/spider
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Search for TaskGraph definition and from_json implementation
cd components/spider-core || exit
rg -n "impl.*TaskGraph|fn from_json" --type=rust -A 5Repository: y-scope/spider
Length of output: 2881
🏁 Script executed:
#!/bin/bash
# Look for error type definitions
cd components/spider-core || exit
rg -n "enum.*Error|struct.*Error|type.*Error" --type=rust | head -30Repository: y-scope/spider
Length of output: 198
🏁 Script executed:
#!/bin/bash
# Check the test file and the specific lines mentioned
cd components/spider-core || exit
cat -n tests/test_task_graph_serde.rs | sed -n '65,110p'Repository: y-scope/spider
Length of output: 1823
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
cat -n src/task.rs | head -50Repository: y-scope/spider
Length of output: 1152
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
rg -n "pub enum Error" -A 20 --type=rustRepository: y-scope/spider
Length of output: 930
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check if serde_json::Error is used elsewhere with structured matching
rg -n "serde_json::Error|\.is_io\(\)|\.is_syntax\(\)|\.is_eof\(\)|\.is_missing_field\(\)" --type=rustRepository: y-scope/spider
Length of output: 175
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check if there's a custom error handling or validation layer
rg -n "custom error|Error::|impl.*From" --type=rust -A 3 | head -60Repository: y-scope/spider
Length of output: 3949
🏁 Script executed:
#!/bin/bash
# Check if serde_json::Error has methods for structured checking
python3 << 'EOF'
import subprocess
result = subprocess.run(['rg', '-n', 'is_io|is_syntax|is_eof|is_missing_field|is_duplicate_field|classification', '--type=rust'],
capture_output=True, text=True, cwd='components/spider-core')
print(result.stdout)
print("---STDERR---")
print(result.stderr)
EOFRepository: y-scope/spider
Length of output: 144
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check what serde_json version is being used
rg -n "serde_json" Cargo.tomlRepository: y-scope/spider
Length of output: 83
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check the actual error message that gets generated for missing fields
# by looking at the visitor implementation more carefully
cat -n src/task/task_graph.rs | sed -n '708,750p'Repository: y-scope/spider
Length of output: 2003
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check if there are any other error handling patterns in the codebase
rg -n "Error::SerdeJsonError|\.classification\(\)" --type=rust -A 2 -B 2Repository: y-scope/spider
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for alternative error matching approaches in Rust projects
# Check if serde_json provides classification or is_* methods
python3 << 'EOF'
# Looking up serde_json 1.0.149 documentation
# Let's check if the error type is accessible and if it has classification methods
import subprocess
result = subprocess.run(['rg', '-n', 'classification|is_io|is_syntax|is_eof', '--type=rust'],
capture_output=True, text=True, cwd='components/spider-core')
print("Results:", result.stdout)
if result.stdout == "":
print("No usage of error classification methods found in codebase")
EOFRepository: y-scope/spider
Length of output: 126
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check if there's a custom deserializer or error handler that could be used
rg -n "struct TaskGraphVisitor|impl.*Visitor" -A 30 --type=rust | head -80Repository: y-scope/spider
Length of output: 2404
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check if there are imports from serde_json in the test file
rg -n "use serde_json|use std::error|use thiserror" tests/test_task_graph_serde.rsRepository: y-scope/spider
Length of output: 40
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Look at the complete test file to understand context better
wc -l tests/test_task_graph_serde.rsRepository: y-scope/spider
Length of output: 92
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check if any other tests use structured error matching or serde_json error methods
rg -n "Error::|is_io|is_syntax|matches!" tests/test_task_graph_serde.rsRepository: y-scope/spider
Length of output: 40
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check serde_json error classification and available methods
cat -n src/task/task_graph.rs | sed -n '240,260p'Repository: y-scope/spider
Length of output: 795
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check how serde_json::Error is handled in practice - check if classification exists
rg -n "serde_json::error|Error::SerdeJsonError" --type=rust -B 2 -A 5Repository: y-scope/spider
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Let's check the serde_json documentation for available error methods
# by searching for any Rust documentation or checking what's available in 1.0.149
python3 << 'EOF'
# According to serde_json 1.0.x documentation, serde_json::Error should have:
# - is_io()
# - is_syntax()
# - is_eof()
# - is_missing_field() (added in 1.0.75)
# - is_duplicate_field() (added in 1.0.104)
# - classification() (returns a Classification enum)
# Let's verify if the code can access these methods
print("serde_json 1.0.149 provides the following error classification methods:")
print("- is_io(): checks if error is an I/O error")
print("- is_syntax(): checks if error is a syntax error")
print("- is_eof(): checks if error is EOF")
print("- is_missing_field(): checks if error is a missing field (serde deserialization)")
print("- is_duplicate_field(): checks if error is a duplicate field")
print("- classification(): returns Classification enum with Io, Syntax, or Eof variants")
print("\nThese allow structured error matching without string matching!")
EOFRepository: y-scope/spider
Length of output: 543
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Verify these methods would work with the Error type wrapping serde_json::Error
# Create a simple test to see if we can match the error variant
cat > /tmp/test_error_matching.rs << 'RUST'
// This demonstrates that we can pattern match on Error::SerdeJsonError
// and then call is_missing_field() on the inner serde_json::Error
match some_result {
Ok(_) => panic!("should fail"),
Err(Error::SerdeJsonError(e)) => {
assert!(e.is_missing_field()); // This is what the review suggests
},
Err(_) => panic!("unexpected error"),
}
RUST
cat /tmp/test_error_matching.rsRepository: y-scope/spider
Length of output: 419
🏁 Script executed:
#!/bin/bash
cd components/spider-core || exit
# Check if there's a way to distinguish custom errors vs missing field errors
cat -n src/task/task_graph.rs | sed -n '718,730p'Repository: y-scope/spider
Length of output: 713
🏁 Script executed:
#!/bin/bash
# Verify the serde_json Error API available in 1.0.149
python3 << 'EOF'
# serde_json::Error in 1.0.149 has the classification() method
# which returns a Classification enum with variants: Io, Syntax, or Eof
# However, serde-json doesn't expose a variant for custom deserialization errors
# The two test cases are:
# 1. missing field - uses de::Error::missing_field() -> creates serde_json error with is_missing_field() = true
# 2. incompatible version - uses de::Error::custom() -> creates serde_json error (no specific method to check)
# For case 2, we'd still need string matching or a custom error type wrapper
print("Analysis:")
print("Test 1 (lines 70-83): Missing field errors")
print(" Current: err.to_string().contains('missing field')")
print(" Better: if let Error::SerdeJsonError(e) = err { assert!(e.is_missing_field()); }")
print("")
print("Test 2 (lines 97-104): Custom error message")
print(" Current: err.to_string().contains('incompatible task graph schema version')")
print(" No structured variant for custom errors in serde_json")
print(" Would need custom error type or continue with string matching")
EOFRepository: y-scope/spider
Length of output: 489
Use serde_json::Error::is_missing_field() instead of string matching for missing field errors (lines 70–83).
For the test at lines 70–83, replace err.to_string().contains("missing field") with a structured check after pattern matching on Error::SerdeJsonError:
match TaskGraph::from_json(&task_graph.to_string()) {
Ok(_) => panic!("deserialization should fail"),
Err(Error::SerdeJsonError(e)) => assert!(e.is_missing_field()),
Err(_) => panic!("unexpected error variant"),
}For the test at lines 97–104, the error is created via de::Error::custom(), which doesn't expose a structured variant in serde_json. Consider introducing a custom error type with variants (e.g., SchemaMismatch) or accept continued string matching for custom validation errors.
🤖 Prompt for AI Agents
In `@components/spider-core/tests/test_task_graph_serde.rs` around lines 70 - 83,
Update the test_missing_fields test to stop asserting on the error string and
instead pattern-match the returned error from TaskGraph::from_json;
specifically, in the Err arm match Error::SerdeJsonError(e) and call
e.is_missing_field() (serde_json::Error::is_missing_field) to verify the
missing-field case, and for any other Err variant panic with "unexpected error
variant"; leave the other test (lines ~97–104) unchanged or consider adding a
custom error variant later since de::Error::custom() cannot be checked via
is_missing_field().
There was a problem hiding this comment.
We converted the error type to our own error, so we are missing the details of what type of json error it is.
There was a problem hiding this comment.
@LinZhihao-723, thank you for the clarification! You're right—if your Error type wraps the serde_json::Error but you don't expose methods to inspect the underlying error category, string matching on the error message is the practical approach.
My suggestion to use is_missing_field() assumed direct access to the serde_json::Error methods, which doesn't align with your error handling design. String matching is a reasonable solution in this case. 👍
| err.to_string() | ||
| .contains("incompatible task graph schema version") |
There was a problem hiding this comment.
Using error message to determine the error type is a code smell. Can we use a custom error enum?
There was a problem hiding this comment.
This is a bit tricky. The type of the source error is a trait, and we had to throw the error as de::Error::custom. And this error is flattened into spider_core::task::Error::RmpSerdeDecodeError or spider_core::task::Error::SerdeJsonError without preserving the original error type info. There's indeed a reason to do so: at the user level, I don't think it matters what exact type of error it is when the given task graph fails to deserialize; in most cases, this is out of scope for the program to handle at runtime. Printing the error in the log is sufficient, so all we need is to preserve enough information in the error message. That said, I wouldn't make the error enum to differentiate all deserialization failure cases since it creates unnecessary maintenance effort where the benefit isn't obvious.
2 participants
Description
This PR adds
serdesupport forTaskGraph, and further adds helpers for serializing/deserializing a task graph in JSON format.Under the hood, the task graph is serialized into a list of
TaskDescriptor. The descriptor contains the necessary information to insert a task into the graph. The API for adding new tasks toTaskGraphhas also been updated to accept aTaskDescriptor.When deserializing from a serializable task graph, it replays the task insertion by feeding the serialized
TaskDescriptorinto the task graph in order.The serde is implemented using low-level APIs for two reasons:
In addition to the task descriptor, we also serialize a task graph schema version as a part of the serialized task graph. This schema version determines whether the current build of
spider-coresupports a serialized task graph. In the future, this schema version may be used to differentiate which serializable task graph format to choose at runtime.The test cases for serde are added under the
testsfolder (not as in-place unit tests). This is to better simulate how the serde feature is used at the user level, ensuring that the top-level API works when the underlying serde helpers are private to the user.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.