fix: propagate reasoner failures to the exit code + extended deep taxonomy benchmark - #1962
Open
jeswr wants to merge 3 commits into
Open
fix: propagate reasoner failures to the exit code + extended deep taxonomy benchmark#1962jeswr wants to merge 3 commits into
jeswr wants to merge 3 commits into
Conversation
Two related changes for the extended-deep-taxonomy issues:
- perf/bench.ts: add an 'extended deep taxonomy benchmark [1000]' case in
the canonical EDT shape (individual at the bottom of a 1000-level
rdfs:subClassOf chain with 3-way branching, backward rule
{?X a ?D} <= {?C rdfs:subClassOf ?D. ?X a ?C}, target membership via
--query). EYE proves it in linear time (<0.5s warm locally), so the CI
benchmark stays bounded. The case deliberately boots a fresh module per
iteration: re-running main() on a pre-loaded module re-asserts the
backward rule and the duplicated clauses make the backward search explode
(documented in the case comment).
- lib/bin/main.ts: the CLI used to exit 0 with empty output when the
reasoner failed (e.g. resource errors on large N3 files, parse errors).
EYE reports failures on stderr with an '** ERROR **' marker while the
underlying Prolog goal still succeeds, so mainFunc now captures stderr,
checks for the marker (and for uncaught Prolog exceptions, which
lib/query.ts qaQuery now surfaces by returning the final call result),
and sets process.exitCode = 1 on failure.
- __tests__/cli-test.ts: regression tests asserting a non-zero exit code
for an erroring input and an untouched exit code on success.
Closes #337
Closes #338
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'EYE JS Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.25.
| Benchmark suite | Current: b387b51 | Previous: d129aee | Ratio |
|---|---|---|---|
Run socrates query |
14.77 ops/sec (±7.81%) |
22.2 ops/sec (±2.39%) |
1.50 |
Executing the socrates query |
119 ops/sec (±22.29%) |
3890 ops/sec (±1.67%) |
32.69 |
Run deep taxonomy benchmark [10] |
7.09 ops/sec (±2.94%) |
18.83 ops/sec (±6.56%) |
2.66 |
Run deep taxonomy benchmark [50] |
0.27 ops/sec (±0.59%) |
19.63 ops/sec (±7.26%) |
72.70 |
Run deep taxonomy benchmark [100] |
0.04 ops/sec (±1.05%) |
18.67 ops/sec (±6.95%) |
466.75 |
Run deep taxonomy benchmark [10] [reasoning only] |
7.7 ops/sec (±16.76%) |
4107 ops/sec (±1.65%) |
533.38 |
Run deep taxonomy benchmark [50] [reasoning only] |
0.63 ops/sec (±23.46%) |
4147 ops/sec (±1.85%) |
6582.54 |
Run deep taxonomy benchmark [100] [reasoning only] |
0.17 ops/sec (±24.99%) |
4136 ops/sec (±1.60%) |
24329.41 |
Run timbl + foaf + rdfs rules |
2.15 ops/sec (±2.18%) |
17.23 ops/sec (±6.18%) |
8.01 |
Run timbl + foaf + owl2rl rules |
1.21 ops/sec (±1.90%) |
17.03 ops/sec (±3.91%) |
14.07 |
Run timbl + rdfs rules |
4.75 ops/sec (±3.74%) |
18.28 ops/sec (±6.60%) |
3.85 |
Run timbl + owl2rl rules |
6.04 ops/sec (±1.03%) |
17.1 ops/sec (±6.85%) |
2.83 |
Run timbl + foaf + rdfs rules [string] |
2.22 ops/sec (±0.50%) |
18.45 ops/sec (±7.08%) |
8.31 |
Run timbl + foaf + owl2rl rules [string] |
1.27 ops/sec (±0.19%) |
18.54 ops/sec (±6.64%) |
14.60 |
Run timbl + rdfs rules [string] |
4.93 ops/sec (±0.43%) |
18.77 ops/sec (±6.77%) |
3.81 |
Run timbl + owl2rl rules [string] |
6.24 ops/sec (±0.26%) |
18.98 ops/sec (±6.47%) |
3.04 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @jeswr
`proc` is dependency-injected into mainFunc so tests can observe the exit code; assigning to it is the point of the change, so silence no-param-reassign for that one line. Lint-only, no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Coverage thresholds were failing on the new exit-code logic: - dist/bin/main.js 64-65: the qaQuery catch block was never executed (its istanbul-ignore hint was emitted by tsc as a trailing comment of the try block, so it was ignored). Rather than fixing the hint, drop it and exercise the path for real: two new CLI tests reject qaQuery with an Error and a non-Error, covering both sides of the `e instanceof Error` ternary and the failed exit-code branch. - dist/transformers.js 86: the "Error while parsing query result" throw in parse() was uncovered; a new test injects a stub SWIPL build whose output is invalid N3 and asserts the rejection. No library behavior changes and no threshold changes; local run is now 100% statements/lines/functions, 98.71% branches (the only remaining miss is the tsc __importDefault helper branch, pre-existing on main). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚧 Draft — agent-generated PR. Written by an AI agent working on the issue backlog; please review before considering it for merge.
Closes #337
Closes #338
Extended deep taxonomy benchmark (#337)
perf/bench.tsgains aRun extended deep taxonomy benchmark [1000]case in the canonical EDT shape (the one from #338 / eulersharp 12dtb): an individual at the bottom of a 1000-levelrdfs:subClassOfchain with 3-way branching, the single backward rule{?X a ?D} <= {?C rdfs:subClassOf ?D. ?X a ?C}., and the target membership asked via--query. EYE proves it in linear time — locally (nice'd, shared 2-core box) a full run including module boot is 0.4–1.4 s and the derived:i1000 a :N1000.was asserted on every validation run, so the CI benchmark job stays bounded.Why there is no
[reasoning only]variant on a pre-loaded module (the pattern the DT[10/50/100] cases use): re-runningmainon the same module re-asserts the backward rule, and the duplicated rule clauses make the backward search explode — measured: first run 843 ms, second run did not terminate within 180 s. benchmark.js re-invokes the case body on the same state, so a pre-loaded-module variant would hang CI. The fresh-module-per-iterationn3reasonercase is immune; boot cost can be subtracted using the existingInitialise SWIPL with EYE imagecase. (This measurement is probably also relevant to any future warm-instance-pool work: module reuse needs state clearing, not just FS cleanup.)Non-zero exit code on reasoner failure (#338)
The CLI used to exit
0with empty output when the reasoner failed — exactly the silent failure in #338. Empirically, EYE reports failures (parse errors, resource errors, missing files) on stderr with an** ERROR **marker while the underlying Prolog goal still succeeds, and on a successful run without--quietit also writes a benign banner/timing to stderr — so "any stderr" is not a usable failure signal, but the marker is.lib/bin/main.ts: captures the module's stderr (still echoing it), checks for the** ERROR **marker and for uncaught Prolog exceptions, and setsprocess.exitCode = 1on failure (normal output flushing still happens; no hardexit()).lib/query.ts:qaQuerynow returns the final call result ({ done, error?, message? }— the shape swipl-wasm produces), so callers can observe an uncaught exception. Backwards compatible (it previously resolvedundefined).__tests__/cli-test.ts: new cases assertexitCode === 1+** ERROR **on stderr for an intentionally invalid input, and an untouchedexitCodeon success.Note: this makes the CLI exit non-zero on inputs that previously "succeeded" silently with no output — intended, but technically observable behaviour change for scripts that relied on exit 0.
Validation
jest __tests__/cli-test.ts: 13/13 pass (11 existing + 2 new).tscbuild + standalone type-check ofperf/bench.tspass.lib/bin/main.ts,lib/query.ts,perf/bench.ts,__tests__/cli-test.tsare touched by fix: Parse quad results from the reasoner and drop the obsolete RDF-star workaround #1950/test: Sample memory after forced GC in the leak tests #1951/test: Make browser e2e tests deterministic and leak-free #1952/test: Add browser dist e2e test and document RDF 1.2 support level #1954/fix!: bump swipl-wasm from 7.0.10 to 8.0.1 #1945.🤖 Generated with Claude Code