[neha-5456]

Summary

Adds configurable error handling to S3Source so document parsing failures can either be raised or skipped without stopping the entire ingestion pipeline.

Fixes #323

Changes

• Added on_error option to S3Source:
  • "raise" — default behavior; parsing errors are propagated.
  • "skip" — logs the parsing failure and continues processing remaining S3 objects.
• Added validation for unsupported on_error values.
• Added warning logging when an object is skipped.
• Ensured S3 access/download errors such as get_object failures are still propagated and are not silently skipped.
• Updated Markdown and CSV decoding so parsing failures follow the configured error-handling policy.
• Added tests covering:
  • Default raise behavior.
  • Skip behavior and continued ingestion.
  • Invalid on_error values.
  • Warning logging.
  • S3 get_object errors.
  • Invalid Markdown front matter.

Testing

uv run --no-sync pytest tests/test_s3_source.py -q

Result: 11 passed

uv run --no-sync ruff check src/dynavec/ingest.py tests/test_s3_source.py

Result: All checks passed!

uv run --no-sync mypy src/dynavec/ingest.py

Result: Success: no issues found in 1 source file

[Kaap10]

Thanks for working on this @neha-5456! The overall approach and test suite are in great shape, but there are a few important issues and acceptance criteria from #323 that need to be resolved before this can be merged:

---

1. src/dynavec/ingest.py (around line 702): Re-raise MissingDependencyError when on_error="skip"

In S3Source.__iter__:

try:
    yield from self._parse_object(key, data, content_type)
except Exception as exc:
    if self.on_error == "skip":
        logger.warning(...)
        continue
    raise

Issue:
_parse_object raises MissingDependencyError if optional libraries like pypdf, python-docx, python-pptx, or openpyxl are not installed. Since MissingDependencyError inherits from DynavecError (which is an Exception), setting on_error="skip" will catch this exception and silently skip every PDF / DOCX file in the bucket as if it were a corrupted document. Missing packages are environment configuration errors, not document parsing errors, and should fail fast.

Suggested Fix:

try:
    yield from self._parse_object(key, data, content_type)
except MissingDependencyError:
    raise
except Exception as exc:
    if self.on_error == "skip":
        logger.warning(
            "Failed to parse S3 object s3://%s/%s (content_type=%s): %s",
            self.bucket,
            key,
            content_type,
            exc,
        )
        continue
    raise ValueError(f"Failed parsing s3://{self.bucket}/{key}: {exc}") from exc

---

2. src/dynavec/ingest.py (around lines 709–712): Include bucket and key in exception under on_error="raise"

Issue:
As noted in the acceptance criteria of #323:

"When on_error="raise" (default): Wrap the underlying parser error in a descriptive ingestion exception (e.g. ValueError(f"Failed parsing s3://{self.bucket}/{key}: {exc}")) so the caller knows exactly which bucket and key caused the failure."

Currently, doing a bare raise emits an uncontextualized exception like UnicodeDecodeError or ScannerError. In a production bucket with thousands of files, the user won't know which object or key caused the failure.

Suggested Fix:
Wrap the error in ValueError (or another descriptive exception) with s3://{self.bucket}/{key} in the message (see snippet in point 1).

---

3. src/dynavec/ingest.py (around lines 665–676): Inconsistent plain text / fallback error handling

In _parse_object:

# 7. Plain text or generic fallback
try:
    text = data.decode("utf-8-sig")
    if text and text.strip():
        yield Record(...)
except UnicodeDecodeError:
    pass

Issue:
try ... except UnicodeDecodeError: return was removed from the Markdown and CSV branches, but left here. If a .txt file has invalid UTF-8 bytes:

• Under on_error="raise", it silently passes rather than raising.
• Under on_error="skip", it silently passes without logging any warning.

Suggested Fix:
Allow decoding in this branch to let the error bubble up to __iter__ so it follows the configured on_error policy.

---

4. src/dynavec/ingest.py (lines 480–502): Type hints and docstring

1. Type annotation: In __init__, please change on_error: str = "raise" to:

   on_error: Literal["raise", "skip"] = "raise"

   (Import Literal from typing), matching existing conventions in dynavec.
2. Docstring: Please document on_error under Parameters in the S3Source docstring.

---

5. tests/test_s3_source.py: Update tests for error message context and MissingDependencyError

Please add/update tests to verify:

1. When on_error="raise", the raised exception contains the bucket name and object key in its message (e.g. s3://test-bucket/documents/broken.md).
2. When on_error="skip", if MissingDependencyError is raised, it is not swallowed and still propagates.

[codeforstartups]

Reviewed and verified locally — clean, well-scoped resilience feature (closes #323). 🎯

S3Source gains on_error="raise"|"skip" (validated up front), with per-object parse failures either propagated or logged-and-skipped so one bad object doesn't abort a whole bucket ingest. I like that the policy wraps only _parse_object — download/access errors (get_object) still propagate, as they should. Bonus: moving the utf-8-sig decode out of a silent except UnicodeDecodeError: return into the policy path means non-UTF8 files are no longer silently dropped — much better default behavior.

Verified locally (ruff clean, strict mypy Success, test_s3_source passing); all 4 CI checks green. Merging — thanks @neha-5456!