fix(docs): fix formatting issues in README.md
- adapt meta-/code-/obs- rules to the local Python benchmark pipeline - replace db-sql-ddl, code-config-env-scope, test-e2e-pytest with pipeline equivalents - add code-python-style, data-determinism, data-naming-units, bench-methodology, build-pipeline-tasks - normalize specs and README typography to ASCII per code-data-formatting - rewrite root CLAUDE.md trigger table; add .cursorrules and .gitignore - specs: pipeline plan and task specs 00-11 - rules: 19 binding rule files adapted for this project - docs: CLAUDE.md rule-trigger table - config: .cursorrules commit convention, .gitignore excluding out/ and .venv/ - docs: rewrite README.md with pipeline diagrams, setup guide, result placeholders - config: requirements.txt for the closed dependency list - datagen: make_lab_config.py writes out/config/lab_config.yaml and .done marker
This commit is contained in:
181
docs/rules/code-error-handling.md
Normal file
181
docs/rules/code-error-handling.md
Normal file
@@ -0,0 +1,181 @@
|
||||
# Code / Error handling - never swallow, always surface, fail loud
|
||||
|
||||
**Purpose + scope.** Governs every `try:` / `except:` / `raise` site in
|
||||
Python code in this repository (data generation, format converters, benchmark
|
||||
runner, extrapolation, reporting, helper tools). Binding for all contributors.
|
||||
|
||||
The pipeline's core failure principle: **a wrong artifact is worse than no
|
||||
artifact.** Every task's output feeds downstream tasks; silently degraded
|
||||
data poisons every result built on it. When in doubt, abort with a clear
|
||||
message and a non-zero exit code.
|
||||
|
||||
---
|
||||
|
||||
## 1. The rule
|
||||
|
||||
1. **Never `except: pass`.** A bare swallow is always a bug. Forbidden
|
||||
absolutely.
|
||||
2. **Never `except Exception: logger.warning("...") + continue`** for
|
||||
anything that was not an EXPECTED, documented, transient condition. If
|
||||
you swallow and continue, you must be able to state why the program can
|
||||
correctly proceed.
|
||||
3. **Always `logger.error(..., exc_info=True)` in `except` blocks.**
|
||||
`exc_info=True` captures the traceback into the log record (see
|
||||
[obs-python-logging.md](obs-python-logging.md)). Without it the original
|
||||
exception is lost.
|
||||
4. **Re-raise unless you have a defined recovery path.** If the caller
|
||||
cannot continue meaningfully without the failed operation, re-raise
|
||||
after logging.
|
||||
5. **Surface to the right channel:**
|
||||
- **CLI script (every pipeline task):** print the error to stderr and
|
||||
exit non-zero. A task that failed MUST NOT write its
|
||||
`./out/.done/<task>.ok` marker (see
|
||||
[build-pipeline-tasks.md](build-pipeline-tasks.md)).
|
||||
- **Validation gate (row counts, checksums, size bounds):** a failed
|
||||
gate is an abort, not a warning. Example: task 03 aborts when the
|
||||
generated tree is outside 120-200 MB; converters abort when row counts
|
||||
do not match `MANIFEST.csv`.
|
||||
- **Library / helper function:** re-raise; let the caller decide.
|
||||
6. **Defensive "this cannot happen" branches must surface, not silently
|
||||
`return`.** If you reach a defensive branch, log at ERROR with the
|
||||
branch name and the inputs that got you there, then raise.
|
||||
|
||||
## 2. The exception ladder - the only three sanctioned swallow patterns
|
||||
|
||||
Code may swallow ONLY in these three documented patterns. Anything else
|
||||
re-raises after logging.
|
||||
|
||||
### Pattern A: documented, recoverable transient (retry)
|
||||
|
||||
```python
|
||||
for attempt in range(MAX_RETRIES):
|
||||
try:
|
||||
return do_thing()
|
||||
except (psycopg.OperationalError, TimeoutError) as exc:
|
||||
logger.warning("attempt %d/%d failed: %s", attempt + 1, MAX_RETRIES, exc)
|
||||
time.sleep(backoff(attempt))
|
||||
# All retries exhausted - surface.
|
||||
logger.error("do_thing failed after %d retries", MAX_RETRIES)
|
||||
raise
|
||||
```
|
||||
|
||||
Justified: the per-attempt warning is acceptable; the final failure
|
||||
re-raises.
|
||||
|
||||
### Pattern B: optional best-effort cleanup
|
||||
|
||||
```python
|
||||
def _close_conn(conn) -> None:
|
||||
try:
|
||||
conn.close()
|
||||
except Exception:
|
||||
# Cleanup is best-effort; the operation already finished, log and continue.
|
||||
logger.warning("closing connection failed", exc_info=True)
|
||||
```
|
||||
|
||||
Justified: the comment names why continuing is correct; the traceback is
|
||||
still captured.
|
||||
|
||||
### Pattern C: benchmark-cell isolation - record and continue the matrix
|
||||
|
||||
```python
|
||||
try:
|
||||
result = run_cell(fmt, query, timeout_s=30 * 60)
|
||||
except CellTimeout:
|
||||
logger.error("cell %s/%s timed out", fmt, query)
|
||||
write_result_row(fmt, query, status="TIMEOUT") # recorded, not hidden
|
||||
except Exception:
|
||||
logger.error("cell %s/%s failed", fmt, query, exc_info=True)
|
||||
write_result_row(fmt, query, status="ERROR")
|
||||
```
|
||||
|
||||
Justified: spec 09 requires the benchmark matrix to survive a failing cell
|
||||
(RDF Q5 is expected to time out). The failure is RECORDED as data
|
||||
(`TIMEOUT` / `ERROR` in `results_raw.csv`), never silently dropped, and the
|
||||
run continues with the next cell. This pattern applies ONLY to the benchmark
|
||||
runner's per-cell boundary - a failure in the runner itself (setup, result
|
||||
writing) still aborts.
|
||||
|
||||
## 3. Loading into SQLite / PostgreSQL - all-or-nothing per table
|
||||
|
||||
A converter that half-loads a table leaves a database that LOOKS complete
|
||||
and silently skews every benchmark run against it.
|
||||
|
||||
1. Wrap each table's bulk load in a transaction; on any error roll back
|
||||
and abort the converter with a non-zero exit.
|
||||
2. After loading, validate row counts against `MANIFEST.csv` and fail loud
|
||||
on any mismatch (see
|
||||
[test-pipeline-validation.md](test-pipeline-validation.md)).
|
||||
3. Never write the task's `.done` marker after a partial load.
|
||||
|
||||
## 4. Anti-patterns
|
||||
|
||||
- **`except: pass`** - forbidden absolutely.
|
||||
- **`except Exception: logger.warning("could not X")` + continue** -
|
||||
forbidden unless the function documents that "could not X" is a
|
||||
recoverable expected outcome. If it is, phrase it "X skipped (expected
|
||||
condition Y)" so a reader does not grep "could not" hunting for bugs.
|
||||
- **Catching `BaseException`** - forbidden (swallows `KeyboardInterrupt`,
|
||||
`SystemExit`). Use `Exception`.
|
||||
- **`logger.error("X")` without `exc_info=True` in an `except` block** -
|
||||
forbidden. The traceback IS the diagnostic.
|
||||
- **`logger.error(str(exc))`** - forbidden; redundant with `exc_info=True`
|
||||
and loses type and stack.
|
||||
- **Returning `None` to signal failure** from a function whose contract is
|
||||
"return T" - forbidden. Either raise, or return `T | None` with
|
||||
documented semantics the caller checks.
|
||||
- **`raise Exception("...")`** - forbidden. Use a specific class:
|
||||
`ValueError` / `RuntimeError` / `KeyError` for trivial cases, a custom
|
||||
domain exception subclass (e.g. `ManifestMismatchError`) for domain
|
||||
conditions.
|
||||
- **`raise` chained without context** - inside an `except`, prefer
|
||||
`raise NewError(...) from exc` so the original cause stays visible in the
|
||||
chained traceback. A bare `raise` that re-raises the same exception is
|
||||
fine; `raise SomethingElse(...)` without `from exc` is forbidden.
|
||||
- **Writing a `.done` marker in a `finally` block** - the marker means
|
||||
"completed successfully", never "attempted".
|
||||
- **Clamping / repairing out-of-range generated values silently** - a
|
||||
physical-model bug must surface, not be masked by a `max(0, ...)` added
|
||||
"to be safe". Deliberate clamps defined by the spec (e.g. COF floor 0.12)
|
||||
are model logic, not error handling.
|
||||
|
||||
## 5. Defensive branches
|
||||
|
||||
```python
|
||||
# Good:
|
||||
def open_store(fmt: str) -> Store:
|
||||
if fmt == "sqlite":
|
||||
return SqliteStore()
|
||||
if fmt == "postgresql":
|
||||
return PgStore()
|
||||
# Defensive: a new format added without updating this factory lands here.
|
||||
logger.error("open_store: unknown format=%r", fmt)
|
||||
raise ValueError(f"unknown storage format: {fmt!r}")
|
||||
```
|
||||
|
||||
The silent-`return None` form lets the unknown value propagate and breaks
|
||||
downstream code with a confusing `AttributeError` on the wrong line.
|
||||
|
||||
## 6. Logging vs surfacing - the decision
|
||||
|
||||
For a caught exception, ask in order:
|
||||
|
||||
1. **Will the pipeline produce a wrong artifact if I continue?** Re-raise /
|
||||
abort.
|
||||
2. **Is this expected and recoverable in the caller's contract?** Log at
|
||||
WARNING with `exc_info=True`, return a sentinel the caller checks.
|
||||
3. **Is this an isolated benchmark cell (pattern C)?** Log at ERROR, record
|
||||
the status as data, continue the matrix.
|
||||
4. **Is the process going down anyway?** Log at CRITICAL with
|
||||
`exc_info=True`, then `sys.exit(1)` or let it propagate.
|
||||
|
||||
## 7. Related documents
|
||||
|
||||
- [obs-python-logging.md](obs-python-logging.md) - logging API and severity
|
||||
levels.
|
||||
- [test-pipeline-validation.md](test-pipeline-validation.md) - the
|
||||
validation gates that turn silent corruption into loud failures.
|
||||
- [build-pipeline-tasks.md](build-pipeline-tasks.md) - `.done` marker
|
||||
semantics.
|
||||
- [bench-methodology.md](bench-methodology.md) - the TIMEOUT contract for
|
||||
benchmark cells.
|
||||
Reference in New Issue
Block a user