Skip to content

Conversation

@soraxas
Copy link

@soraxas soraxas commented Jan 5, 2026

This PR refactors the spz and spz-pywrapper crates:

  • point spz-pywrapper/Cargo.toml to the spz crate in the same repo
  • remove dependencies on anyhow and tokio, introduces a custom error type (SpzError), and cleans up the codebase to use more explicit error handling
  • Removed unnecessary dependencies on anyhow, tokio, libc, rayon, and clap (probably got auto-pulled in with LLM due to your AGENTS.md? (as they are mentioned in the md, but never needed in the actual code base)
    • this reduce spz's deps from ~91 to ~49 on my machine
  • we don't need to pull the whole async runtime tokio setup (as there's also multiple runtime exists out there); we only need the trait to support async reading

The arbitary derive is hidden behind feature as it's only needed for testing

Signed-off-by: Tin Lai <tin@tinyiu.com>
Signed-off-by: Tin Lai <tin@tinyiu.com>
Signed-off-by: Tin Lai <tin@tinyiu.com>
@Jackneill Jackneill self-requested a review January 7, 2026 14:05
@Jackneill
Copy link
Owner

Jackneill commented Jan 7, 2026

  • Thank you for the pull request.

  1. Please resolve the merge conflicts first and pull in main.
  2. bin/spz.rs has errors.
  3. error handling: you left context out with thiserror when using the SpzError type, whereas with anyhow they were there. i see SpzError type has them, why not use them?
    1. example: thiserror doc .
  4. ci has errors

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR significantly reduces dependencies in the spz crate by removing anyhow, tokio, libc, rayon, clap, rustversion, and serde_json, reducing the total dependency count from ~91 to ~49. The changes introduce a custom error type (SpzError) for more explicit error handling, replace the full tokio async runtime with lightweight futures traits, and make the arbitrary dependency optional behind a fuzzing feature flag.

Key changes:

  • Introduced custom SpzError type replacing anyhow::Error for explicit error handling
  • Replaced tokio async runtime with futures traits for async I/O operations
  • Made arbitrary dependency optional, controlled by the fuzzing feature
  • Updated builder API to accept file path in load() method instead of storing it in the builder

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
crates/spz/Cargo.toml Removed unnecessary dependencies (anyhow, tokio, clap, rayon, libc, rustversion, serde_json); made arbitrary optional
crates/spz/src/errors.rs Added custom SpzError enum with specific error variants
crates/spz/src/lib.rs Exported SpzError in prelude
crates/spz/src/packed.rs Migrated from anyhow to SpzError; added conditional arbitrary derives
crates/spz/src/unpacked.rs Made arbitrary derive conditional on fuzzing feature
crates/spz/src/header.rs Simplified error handling with SpzError
crates/spz/src/gaussian_splat.rs Replaced tokio with futures traits; updated builder API to accept filepath in load()
crates/spz/src/coord.rs Made arbitrary derive conditional on fuzzing feature
crates/spz/src/compression.rs Migrated to SpzError with automatic io::Error conversion
crates/spz/src/mmap.rs Updated error handling to use SpzError
crates/spz/tests/test.rs Updated to use new builder API pattern
crates/spz/examples/load_spz.rs Updated examples for new API and async patterns
crates/spz/benches/util/mod.rs Removed async benchmark helper; updated for new API
crates/spz/benches/benchmarks/load.rs Removed tokio-based async benchmark
crates/spz-pywrapper/Cargo.toml Changed to use local path dependency instead of crates.io version
crates/spz-pywrapper/src/lib.rs Updated module paths to match new spz crate structure
fuzz/Cargo.toml Enabled fuzzing feature for spz dependency
Cargo.lock Reflected removal of unnecessary transitive dependencies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +15 to +16
Mmap::map(&infile).map_err(|_| {
SpzError::LoadPackedError("unable to open file with mmap()".to_string())
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original error from Mmap::map() is being discarded here. This loses valuable information like the specific I/O error that occurred (e.g., permission denied, out of memory). Consider wrapping the io::Error or using a more specific error variant that preserves the underlying cause.

Suggested change
Mmap::map(&infile).map_err(|_| {
SpzError::LoadPackedError("unable to open file with mmap()".to_string())
Mmap::map(&infile).map_err(|e| {
SpzError::LoadPackedError(format!("unable to open file with mmap(): {e}"))

Copilot uses AI. Check for mistakes.
Comment on lines +139 to +141
crate::compression::gzip_to_bytes(data, &mut decompressed).map_err(|_| {
SpzError::LoadPackedError("unable to decompress gzip data".to_string())
})?;
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying I/O error from gzip decompression is being discarded. Since SpzError has an IoError variant with #[from], you should preserve the original error by using a different error variant that wraps io::Error or by adding more specific error types that can preserve the cause chain.

Suggested change
crate::compression::gzip_to_bytes(data, &mut decompressed).map_err(|_| {
SpzError::LoadPackedError("unable to decompress gzip data".to_string())
})?;
crate::compression::gzip_to_bytes(data, &mut decompressed)?;

Copilot uses AI. Check for mistakes.
Comment on lines +143 to +147
let packed: PackedGaussians = decompressed.try_into().map_err(|_| {
SpzError::LoadPackedError(
"unable to parse packed gaussian data".to_string(),
)
})?;
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specific SpzError from the try_into conversion is being discarded and replaced with a generic message. The underlying error likely contains useful details about why parsing failed (e.g., invalid magic number, unsupported version). Consider preserving the original error instead of discarding it.

Suggested change
let packed: PackedGaussians = decompressed.try_into().map_err(|_| {
SpzError::LoadPackedError(
"unable to parse packed gaussian data".to_string(),
)
})?;
let packed: PackedGaussians = decompressed.try_into()?;

Copilot uses AI. Check for mistakes.
}

pub fn to_packed_gaussians(&self, pack_opts: &PackOptions) -> Result<PackedGaussians> {
pub fn to_packed_gaussians(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

pub fn to_packed_gaussians(&self, pack_opts: &PackOptions) -> Result<PackedGaussians> {
pub fn to_packed_gaussians(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let infile = File::open(&file)?;

unsafe { Mmap::map(&infile).with_context(|| "unable to open file with mmap()") }
unsafe {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants