Skip to content

Commit 5708ec0

Browse files
rootcoder007claude
andcommitted
fix: close retest findings N1-N5 (pipeline exit code, cpads path, xgbst inference, verifier regex)
N2: execute_pipeline now tracks per-module failures, prints "FAILED" (not "Done") for a module that errored, and returns 1 when any module failed — previously it always printed "completed successfully" and returned 0, so CI and humans could not tell a 7/23 run from a 23/23 one. N1: the Python bridge resolves --cpads-csv (and --output-dir) to absolute paths before launching Rscript with cwd=output_dir; the R side's fragile 10-level parent-directory walk is removed in favour of resolve-or-error. A relative --cpads-csv with --output-dir outside the repo silently killed all 16 R-backed modules. N4: drop the `is.integer(y)` clause from xgbst task inference — count outcomes are integers and must resolve to regression, not be auto-routed to a binary classifier. Binary is already caught by `all(y %in% c(0,1))`. N3: the gbm fallback hands bernoulli a numeric {0,1}, not a factor (matching the xgboost branch), so classification actually fits for users without xgboost (Suggests-only). N5: anchor the r-squared verifier pattern so bare `r2` no longer matches the substring in column names like `factor2`. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Vansh Singh Ruhela (rootcoder007) <vsruhela@proton.me>
1 parent 3d28a68 commit 5708ec0

5 files changed

Lines changed: 40 additions & 19 deletions

File tree

r-package/morie/R/modules.R

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -75,22 +75,18 @@ morie_list_morie_modules <- function() {
7575
}
7676

7777
.resolve_cpads_csv <- function(cpads_csv) {
78+
# The Python bridge passes an absolute path (see modules.py, finding N1);
79+
# a direct Rscript caller is responsible for a path valid from its own cwd.
80+
# No parent-directory walk: it only ever "worked" when the output dir
81+
# happened to sit inside the repo, and silently surprised everyone else.
7882
if (file.exists(cpads_csv)) {
7983
return(normalizePath(cpads_csv, mustWork = TRUE))
8084
}
81-
current <- normalizePath(getwd(), winslash = "/", mustWork = TRUE)
82-
for (i in seq_len(10L)) {
83-
candidate <- file.path(current, cpads_csv)
84-
if (file.exists(candidate)) {
85-
return(normalizePath(candidate, mustWork = TRUE))
86-
}
87-
parent <- dirname(current)
88-
if (identical(parent, current)) {
89-
break
90-
}
91-
current <- parent
92-
}
93-
stop("CPADS CSV not found: ", cpads_csv, call. = FALSE)
85+
stop(
86+
"CPADS CSV not found: ", cpads_csv,
87+
"\n Pass an absolute path (or one valid from the current working directory).",
88+
call. = FALSE
89+
)
9490
}
9591

9692
#' Canonicalize raw CPADS PUMF columns

r-package/morie/R/xgbst.R

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,10 @@ morie_xgboost_objective <- function(x, y, n_estimators = 100L, learning_rate = 0
3434
if (is.null(dim(x))) x <- matrix(x, ncol = 1)
3535
x <- as.matrix(x)
3636
if (identical(task, "auto")) {
37-
task <- if (is.factor(y) || all(y %in% c(0L, 1L)) || is.integer(y)) {
37+
# A 0/1 vector (integer or double) is caught by the `%in%` test; do NOT
38+
# treat every integer as classification — count outcomes are integers too
39+
# and must resolve to regression (N4).
40+
task <- if (is.factor(y) || all(y %in% c(0L, 1L))) {
3841
"classification"
3942
} else {
4043
"regression"
@@ -79,7 +82,9 @@ morie_xgboost_objective <- function(x, y, n_estimators = 100L, learning_rate = 0
7982
if (!requireNamespace("gbm", quietly = TRUE)) {
8083
stop("install 'xgboost' (preferred) or 'gbm' for morie_xgboost_objective")
8184
}
82-
yv <- if (task == "classification") factor(y) else as.numeric(y)
85+
# gbm's bernoulli requires numeric {0,1}, NOT a factor (N3). Use the same
86+
# coercion as the xgboost branch above so the fallback actually fits.
87+
yv <- if (task == "classification") as.numeric(as.factor(y)) - 1 else as.numeric(y)
8388
df <- as.data.frame(x)
8489
df$.y <- yv
8590
distribution <- if (task == "classification") "bernoulli" else "gaussian"

src/morie/inspector.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@
4343
r"|^ate$|^att$|^atc$|point[_.\-]?est|post[_.\-]?mean)",
4444
re.IGNORECASE,
4545
)
46-
_R_SQUARED_PATTERNS = re.compile(r"(r[_.\-]?squared|r2|rsq|pseudo[_.\-]?r2|adj[_.\-]?r2)", re.IGNORECASE)
46+
_R_SQUARED_PATTERNS = re.compile(
47+
r"(^|[_.\-])(r[_.\-]?squared|r2|rsq|pseudo[_.\-]?r2|adj[_.\-]?r2)($|[_.\-])",
48+
re.IGNORECASE,
49+
) # anchored so bare `r2` no longer matches the substring in `factor2`/`predictor2` (N5)
4750
_SAMPLE_SIZE_PATTERNS = re.compile(r"(^n$|^n[_.\-]obs|sample[_.\-]?size|^nobs$|n_total)", re.IGNORECASE)
4851
_AIC_BIC_PATTERNS = re.compile(r"(^aic$|^bic$)", re.IGNORECASE)
4952

src/morie/modules.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,8 +417,12 @@ def _run_r_module(
417417
if output_dir is None:
418418
_tmp_ctx = tempfile.TemporaryDirectory(prefix=f"morie-{module_name}-")
419419
output_dir = Path(_tmp_ctx.name)
420-
output_dir = Path(output_dir)
420+
output_dir = Path(output_dir).expanduser().resolve()
421421
output_dir.mkdir(parents=True, exist_ok=True)
422+
# Rscript runs with cwd=output_dir, so any relative path we forward would
423+
# resolve against the OUTPUT dir, not the caller's cwd. Make cpads_csv
424+
# absolute here so it works regardless of where output lands (N1).
425+
cpads_csv = Path(cpads_csv).expanduser().resolve()
422426
try:
423427
if not _R_MODULE_SHIM.exists():
424428
raise RuntimeError(

src/morie/runner.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ def execute_pipeline(
9393
except ImportError:
9494
pass
9595

96+
failed: dict[str, str] = {}
9697
for idx, module_name in enumerate(selected, start=1):
9798
if _pbar:
9899
_pbar.desc = f"Running: {module_name}"
@@ -105,12 +106,15 @@ def execute_pipeline(
105106
dataset_key=dataset_key,
106107
output_dir=output_dir,
107108
)
109+
status = "Done"
108110
except Exception as exc:
111+
failed[module_name] = str(exc)
109112
print(f" ERROR in {module_name}: {exc}")
113+
status = "FAILED"
110114
if _pbar:
111115
_pbar.update()
112116
else:
113-
print(f"[{idx}/{total}] Done: {module_name}", flush=True)
117+
print(f"[{idx}/{total}] {status}: {module_name}", flush=True)
114118

115119
if _manager:
116120
_manager.stop()
@@ -123,7 +127,16 @@ def execute_pipeline(
123127
except Exception: # pragma: no cover
124128
pass
125129

126-
print("Pipeline completed successfully.")
130+
n_ok = len(results)
131+
if failed:
132+
print(
133+
f"Pipeline completed {n_ok}/{total} "
134+
f"({len(failed)} failed: {', '.join(failed)})."
135+
)
136+
if results:
137+
print("Succeeded modules:", ", ".join(results.keys()))
138+
return 1
139+
print(f"Pipeline completed successfully ({n_ok}/{total}).")
127140
print("Completed modules:", ", ".join(results.keys()))
128141
return 0
129142

0 commit comments

Comments
 (0)