add e2e integration test#187
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 18 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (36)
WalkthroughAdds an E2E GitHub Actions workflow, converts packaging to pyproject.toml + tool.uv (console script), refreshes docs/CONTRIBUTING to use uv-based development, and applies widespread formatting/config updates across configs, data pipeline, imputation/models, training, tuning, run utilities, and scripts. ChangesRepository setup, packaging, and CI
Documentation and contribution guidance
Config, experiments, and tuning
Data pipeline, preprocessing, and splitting
Imputation implementations and layers
Models, training, metrics, and wrappers
Run, tuning, wandb, and utilities
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-integration.yml:
- Around line 52-59: The current workflow only asserts that
../yaib_logs/mimic_demo exists but not that run.py and create_run_dir() produced
the expected nested layout (mimic_demo/Mortality24/XGBClassifier/<timestamp>);
change the check to validate the full run directory shape by ensuring there is
at least one leaf timestamped run under ../yaib_logs/mimic_demo with the
expected two intermediate folders (e.g., check for a path matching
../yaib_logs/mimic_demo/*/*/* and that the final leaf looks like a timestamp),
or use find with a specific maxdepth/min depth pattern to assert a depth-3
directory exists; reference run.py and create_run_dir() when locating the
expected structure.
- Around line 11-17: The workflow job e2e-integration-test exposes GITHUB_TOKEN
unnecessarily and uses unpinned action versions and weak log verification;
update the job to add a minimal permissions stanza (explicit least-privilege for
required scopes), set actions/checkout@v4 step to persist-credentials: false,
replace version-tagged actions (e.g., actions/setup-python@v5, actions/cache@v4,
actions/upload-artifact@v4, actions/checkout@v4) with pinned commit SHAs, and
strengthen the post-run verification step that currently checks for
"../yaib_logs/mimic_demo" to assert specific output files and/or expected
content checks (e.g., exact log files, model artifact names or key strings) so
the workflow only grants needed token scopes, does not persist credentials to
the workspace, and verifies actual outputs.
- Line 17: Replace floating GitHub Action tags with pinned full commit SHAs:
update the uses entries for actions/checkout@v4, actions/setup-python@v5,
actions/cache@v4, and actions/upload-artifact@v4 to their corresponding
full-length commit SHAs (e.g., actions/checkout@<full-sha>) in
.github/workflows/e2e-integration.yml; fetch the official commit SHAs from each
action's GitHub repo releases/tags, substitute the tag with the full SHA, and
confirm the workflow still runs successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2edafcc0-9ab6-4dc9-94f0-bdb094a5ed80
📒 Files selected for processing (1)
.github/workflows/e2e-integration.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
252-265:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEvaluation command likely uses outdated CLI syntax.
This section uses
uv run icu-benchmarks --eval ..., butPAPER.mdnow documentsuv run icu-benchmarks evaluate .... If--evalis no longer supported, this README path will fail for users following it. Please align on one canonical invocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 252 - 265, The README example uses the outdated flag "--eval"; update the command to use the current subcommand form "evaluate" (i.e., replace "uv run icu-benchmarks --eval" with "uv run icu-benchmarks evaluate") and verify that the rest of the flags (e.g., -d, -n, -t, -m, --generate_cache, --load_cache, -s, -l, -sn, --source-dir) match the current CLI documented in PAPER.md; adjust any renamed flags to their current names so the full invocation (the example command string) runs with the current CLI.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-integration.yml:
- Around line 19-25: Replace the floating action and the floating Python patch
selection: pin the GitHub Action by replacing "astral-sh/setup-uv@v4" and its
"version: \"latest\"" with a fixed release tag or full SHA (e.g. a specific
v4.x.y or commit SHA) and change the uv install step "uv python install 3.10" to
install an exact patch (e.g. "3.10.<patch>") or use an explicit python-version
key set to the exact patch (3.10.<patch>) so the workflow is deterministic;
update the two locations referring to the action and the uv python install
command accordingly.
In `@CONTRIBUTING.md`:
- Around line 20-21: CONTRIBUTING.md currently instructs "uv python install
3.12" while PAPER.md recommends Python 3.10 — pick a single baseline (suggest
standardizing to Python 3.10), update the text in CONTRIBUTING.md to "uv python
install 3.10" (and any adjacent setup lines like "uv sync --dev" if they mention
the version), and ensure PAPER.md, CONTRIBUTING.md and any CI/config files
consistently reference that same Python version; search for other occurrences of
"3.12" or "3.10" in the repo and align them to the chosen baseline.
In `@pyproject.toml`:
- Around line 59-65: The dev extras list in [project.optional-dependencies].dev
is missing ruff while [dependency-groups].dev includes it, causing drift; update
the [project.optional-dependencies].dev entry (the dev extra) to include "ruff"
so both definitions match exactly (ensure the package name and version spec used
in [dependency-groups].dev is mirrored in the dev extra) and run a quick
sync/check to confirm pip install .[dev] and uv sync --group dev produce
identical environments.
- Around line 28-33: The pyproject currently pins torch==2.6.0+cu118 in
project.dependencies which requires the uv-only PyTorch index and breaks plain
pip installs; move the CUDA-specific requirement out of core dependencies into
an optional GPU extra/group (e.g., add an optional dependency group named "gpu"
or "extras.gpu" that contains "torch==2.6.0+cu118") and keep the core dependency
as the PyPI-compatible "torch==2.6.0"; also add a comment/documentation string
near that gpu extra explaining that users must pass --index-url
https://download.pytorch.org/whl/cu118 (or configure the same source in uv) to
install the CUDA wheel; finally, add "ruff" to the project optional dev
dependencies (project.optional-dependencies.dev) to match the
dependency-groups.dev entry so tools and consumers see the same dev tooling.
In `@README.md`:
- Line 286: The README contains a case-sensitive link target "[contribution
guidelines](CONTRIBUTING.MD)" that points to the wrong filename; update the link
target to the actual file name "CONTRIBUTING.md" (leave the link text as-is) so
the anchor matches the repository file; search for the string "CONTRIBUTING.MD"
in README.md and replace it with "CONTRIBUTING.md" to fix the broken link.
---
Outside diff comments:
In `@README.md`:
- Around line 252-265: The README example uses the outdated flag "--eval";
update the command to use the current subcommand form "evaluate" (i.e., replace
"uv run icu-benchmarks --eval" with "uv run icu-benchmarks evaluate") and verify
that the rest of the flags (e.g., -d, -n, -t, -m, --generate_cache,
--load_cache, -s, -l, -sn, --source-dir) match the current CLI documented in
PAPER.md; adjust any renamed flags to their current names so the full invocation
(the example command string) runs with the current CLI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 54b9bbf6-b580-4cb5-b3cc-dc97721fac08
📒 Files selected for processing (9)
.github/workflows/e2e-integration.ymlCONTRIBUTING.MDCONTRIBUTING.mdPAPER.mdREADME.mdenvironment.ymlpyproject.tomlsetup.cfgsetup.py
💤 Files with no reviewable changes (4)
- environment.yml
- setup.cfg
- CONTRIBUTING.MD
- setup.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/adding_model/instructions.md (1)
19-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit language tags to fenced code blocks.
These fenced blocks are missing a language identifier, which triggers markdownlint (MD040) and reduces readability in rendered docs.
Proposed patch
-``` +```bash icu-benchmarks train \ -d demo_data/mortality24/mimic_demo \ # Insert cohort dataset here @@ -``` +``` -``` +```gin # Hyperparameters for TFT model. @@ -``` +``` -``` +```bash icu-benchmarks train \ @@ -``` +```Also applies to: 171-193, 199-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adding_model/instructions.md` around lines 19 - 30, Several fenced code blocks lack language identifiers; update each block containing shell invocations like the "icu-benchmarks train ..." commands to use ```bash and the block with "Hyperparameters for TFT model." to use ```gin (or other appropriate language) so markdownlint MD040 is satisfied and rendered output improves; search for the unlabeled fences around the "icu-benchmarks train" examples and the "Hyperparameters for TFT model." section and add the correct triple-backtick language tags.icu_benchmarks/models/train.py (1)
107-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard optional
dataset_nameslookups to avoidKeyError.When
dataset_namesis omitted, Line 108 sets{}, but Line 129/132/239 still usedataset_names["train"]/["val"]/["test"]. That raisesKeyErroron the default path.Suggested fix
- train_dataset = dataset_class( - data, split=DataSplit.train, ram_cache=ram_cache, name=dataset_names["train"] - ) + train_dataset = dataset_class( + data, + split=DataSplit.train, + ram_cache=ram_cache, + name=dataset_names.get("train", ""), + ) - val_dataset = dataset_class( - data, split=DataSplit.val, ram_cache=ram_cache, name=dataset_names["val"] - ) + val_dataset = dataset_class( + data, + split=DataSplit.val, + ram_cache=ram_cache, + name=dataset_names.get("val", ""), + ) @@ - test_dataset = dataset_class( - data, split=test_on, name=dataset_names["test"], ram_cache=ram_cache - ) + test_dataset = dataset_class( + data, + split=test_on, + name=dataset_names.get("test", ""), + ram_cache=ram_cache, + )Also applies to: 128-133, 238-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@icu_benchmarks/models/train.py` around lines 107 - 109, The code sets dataset_names = {} when omitted but later does direct lookups dataset_names["train"], dataset_names["val"], and dataset_names["test"], which can raise KeyError; update the accesses in train.py to use safe retrieval (e.g., dataset_names.get("train", <fallback>), or pre-populate dataset_names with default keys) wherever dataset_names["train"]/["val"]/["test"] are used (references: dataset_names variable and the direct indexing occurrences dataset_names["train"], dataset_names["val"], dataset_names["test"]) so the function works when dataset_names is not provided..github/workflows/e2e-integration.yml (1)
35-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale error path and dangling line-continuation in the train step.
Two cleanups in this block:
- Line 47: the error message still references
../yaib_logs/mimic_demo, but the check (line 46) and the-lflag (line 42) now useyaib_logs/. Update the message to avoid misleading failure output.- Line 42: the final
-l yaib_logs/ \ends with a trailing backslash followed by a blank line, leaving a dangling continuation. It works today but is fragile—drop the trailing\.🩹 Proposed fix
--seed 2222 \ - -l yaib_logs/ \ + -l yaib_logs/if [ ! -d "yaib_logs/mimic_demo" ]; then - echo "ERROR: Expected log directory ../yaib_logs/mimic_demo was not created." + echo "ERROR: Expected log directory yaib_logs/mimic_demo was not created." exit 1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-integration.yml around lines 35 - 51, Update the train command and verification block: remove the dangling line-continuation backslash after the -l yaib_logs/ flag in the uv run icu-benchmarks train command and change the echo error message that currently references "../yaib_logs/mimic_demo" to reference "yaib_logs/mimic_demo" (or match the CHECK path used by the if statement) so the log-path in the error output matches the actual check; locate the train invocation (uv run icu-benchmarks train and its -l yaib_logs/ flag) and the verification echo that prints the missing directory to apply these fixes.icu_benchmarks/tuning/hyperparameters.py (1)
370-371:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading log: warning says "does not exist" inside the exists branch.
This branch is entered only when
checkpoint.exists()isTrue, yet the warning states the checkpoint does not exist. This contradicts the actual state and the subsequentLoading checkpoint at {checkpoint}info log. The line appears to be stale copy from the scikit-optimize path.🩹 Proposed fix
if checkpoint and checkpoint.exists(): - logging.warning(f"Hyperparameter checkpoint {checkpoint} does not exist.") + logging.info(f"Hyperparameter checkpoint {checkpoint} found.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@icu_benchmarks/tuning/hyperparameters.py` around lines 370 - 371, The log message is incorrect: inside the branch that checks checkpoint.exists() you call logging.warning with "does not exist." Change this to reflect the true state — replace the logging.warning call with logging.info (or remove the warning) and update the message to something like "Hyperparameter checkpoint {checkpoint} exists; loading checkpoint" so it matches checkpoint.exists() and the subsequent "Loading checkpoint at {checkpoint}" info log; edit the statement that references checkpoint.exists(), logging.warning, and the subsequent "Loading checkpoint at {checkpoint}" log accordingly.
🧹 Nitpick comments (1)
icu_benchmarks/run_utils.py (1)
237-245: ⚡ Quick winDuplicate SHAP concat/write — the file is written twice.
The
if shap_values_test:block (Lines 237-239) and the followingtryblock (Lines 241-245) perform the identicalpl.concat(...)+write_parquet(...)to the same path. This writesaggregated_shap_values.parquettwice on every run, and the first block has no error handling. Theif-guarded block looks redundant given thetry/except.♻️ Proposed deduplication
- if shap_values_test: - shap_values = pl.concat(shap_values_test) - shap_values.write_parquet(log_dir / "aggregated_shap_values.parquet") - - try: - shap_values = pl.concat(shap_values_test) - shap_values.write_parquet(log_dir / "aggregated_shap_values.parquet") - except Exception as e: - logging.error(f"Error aggregating or writing SHAP values: {e}") + if shap_values_test: + try: + shap_values = pl.concat(shap_values_test) + shap_values.write_parquet(log_dir / "aggregated_shap_values.parquet") + except Exception as e: + logging.error(f"Error aggregating or writing SHAP values: {e}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@icu_benchmarks/run_utils.py` around lines 237 - 245, Remove the redundant unconditional concat/write block and keep a single guarded operation that both checks shap_values_test and handles errors: replace the two blocks with one try/except that first checks "if shap_values_test:" then calls pl.concat(shap_values_test) and shap_values.write_parquet(log_dir / "aggregated_shap_values.parquet"), and logs any exception; reference the existing symbols shap_values_test, pl.concat, write_parquet, and log_dir/"aggregated_shap_values.parquet" to locate and update the duplicate code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/code-quality.yml:
- Around line 9-27: The workflow job "code-quality" lacks an explicit
permissions block and uses floating action versions; add a least-privilege
permissions: block (e.g., read-only for contents, packages, pull-requests as
needed) to the "code-quality" job and pin the third-party actions referenced
(actions/checkout@v4, actions/setup-python@v5, pre-commit/action@v3.0.1) to
immutable commit SHAs per the project's supply-chain policy; update the job
definition around the "code-quality" job and the steps using those action names
to reference the SHAs and include the minimal permissions.
In @.github/workflows/python-build.yml:
- Around line 48-50: The trusted-publishing environment URL currently points to
the wrong PyPI project ("recipies") and still has the template placeholder;
update the environment block (environment: name: pypi) to use the correct
project name from pyproject.toml (yaib) by changing url to
https://pypi.org/p/yaib and remove the leftover "Replace <package-name>..."
comment so the trusted publishing target matches the actual package.
- Around line 6-39: The build job is missing a least-privilege permissions block
and action refs are not pinned; add a permissions: block with contents: read
scoped to the build job (or top-level) to restrict the GITHUB_TOKEN, and pin the
actions used in the build job (actions/checkout@v4, actions/setup-python@v5,
actions/upload-artifact@v4) to immutable commit SHAs (replace the tag refs with
the corresponding commit SHA for each action) so the 'build' job only has
contents: read and uses pinned action SHAs.
- Around line 23-27: The workflow is injecting untrusted GitHub context directly
into shell scripts (e.g., the "Debug git and version info" step echoing ${{
github.ref }} and the steps calling gh release create/upload with ${{
github.ref_name }}/${{ github.repository }}); instead, add env: entries (e.g.,
REF="$GITHUB_REF" or REF_NAME="$GITHUB_REF_NAME" and REPO="$GITHUB_REPOSITORY")
to the same steps and replace the inline ${ { github.* } } usages inside run:
with the safe shell variables referenced as "$REF" / "$REF_NAME" / "$REPO"
(ensure you quote the variables), updating the gh release create / gh release
upload commands and the debug echo to use these env vars.
In @.pre-commit-config.yaml:
- Line 4: The global top-level exclude pattern `exclude: ".ipynb"` is an
unanchored regex that disables all hooks (including the `nbstripout` hook);
update the config by either anchoring the pattern (e.g., require a suffix or use
^.*\.ipynb$) or remove the top-level `exclude` and place a scoped `exclude` only
on the specific hooks you want to skip (for example the formatting/lint hooks),
ensuring the `nbstripout` hook remains able to see .ipynb files; look for the
top-level `exclude` key and the `nbstripout` hook name to make this change.
In `@docs/adding_model/instructions.md`:
- Line 165: Update the documentation to reference the correct implementation
file icu_benchmarks/models/wrappers.py (not models/wrapper.py) and point readers
to the step_fn method in that file so they inspect and ensure data/device
transfer logic is implemented there; revise the sentence to mention
icu_benchmarks/models/wrappers.py and the step_fn function explicitly.
- Line 128: Remove the duplicated phrase "In this In this" from the sentence in
the documentation and replace it with a single "In this case," so the sentence
reads: "The next step is to use the building blocks defined in layers.py or
modules from an existing library to add to the model in `models/dl_models.py`.
In this case, we use the Pytorch-forecasting library
(https://github.com/jdb78/pytorch-forecasting):" Locate the sentence containing
"In this In this case" and edit it to the corrected single phrase.
In `@docs/adding_model/rnn.py`:
- Around line 14-20: In docs/adding_model/rnn.py, reorder the arguments in the
super().__init__ call to match the runtime implementation: place *args (and
**kwargs if present) before the explicit keyword parameters (input_size,
hidden_dim, layer_dim, num_classes) so the call mirrors
icu_benchmarks/models/dl_models/rnn.py and avoids Ruff B026; locate the
super().__init__ inside the RNN class __init__ and move the *args/*kwargs ahead
of the named keyword arguments.
In `@icu_benchmarks/data/loader.py`:
- Around line 272-285: The constructor/initializer for CommonPandasDataset is
emitting the same DeprecationWarning twice; remove the duplicate warnings.warn
call so the deprecation message is only emitted once during initialization.
Locate the block in CommonPandasDataset (around the vars type check/validation)
and keep a single warnings.warn("CommonPandasDataset is deprecated. Use
CommonPolarsDataset instead.", DeprecationWarning, stacklevel=2) either before
or after the vars type-check (but not both), and delete the redundant call so
tests relying on warning counts no longer fail.
In `@icu_benchmarks/data/preprocessor.py`:
- Around line 763-765: PandasImputationPreprocessor calls self.save_cache and
self.load_cache in apply() (via apply_recipe_to_splits) but its __init__ never
sets these attributes or calls Preprocessor.__init__; initialize the cache flags
by either calling super().__init__(...) from
PandasImputationPreprocessor.__init__ or explicitly set self.save_cache and
self.load_cache (and any other expected Preprocessor attributes) during
construction so apply() can safely access them.
In `@icu_benchmarks/data/split_process_data.py`:
- Around line 201-203: In the complete_train branch the call to
make_train_val_polars(...) uses the raw data variable instead of the
already-produced sanitized_data, skipping sanitization/deduplication and causing
inconsistent splits; change the argument passed to make_train_val_polars in that
branch from data to sanitized_data so both branches use the same sanitized input
(refer to make_train_val_polars and sanitized_data in the function containing
the complete_train branch).
In `@icu_benchmarks/imputation/diffwave.py`:
- Around line 419-421: The residual stacking is wrong: each block is called with
the original `noise` instead of the evolving hidden state `h`, so update the
call to self.residual_blocks[n] to pass the current hidden state `h` (alongside
`conditional` and `diffusion_step_embed`) and assign the returned new hidden
state and skip connection to `h, skip_n` so subsequent residual blocks receive
the updated `h` (symbols to change: residual_blocks, h, skip_n,
self.residual_blocks[n], noise, conditional, diffusion_step_embed).
In `@icu_benchmarks/models/wrappers.py`:
- Around line 580-587: The debug message incorrectly references
"row_indicators.csv" while the code actually saves "pred_indicators.csv"; in the
block that calls np.savetxt (and constructs Path(self.logger.save_dir) /
"pred_indicators.csv"), update the logging.debug call to use the same
filename/path (e.g., Path(self.logger.save_dir) / "pred_indicators.csv" or the
variable used for the save path) so the logged path matches the saved file.
In `@scripts/evaluate_results/aggregrate_experiment.py`:
- Around line 9-17: The default parameter uses a mutable list named models —
change the function signature to use models=None and inside the function set
models =
["LSTM","Transformer","GRU","RNN","LogisticRegression","LGBMClassifier","TCN"]
if models is None; this ensures each call gets a fresh list and avoids the
mutable-default anti-pattern while preserving the original default values (refer
to the models parameter in the function definition where the list is currently
used).
---
Outside diff comments:
In @.github/workflows/e2e-integration.yml:
- Around line 35-51: Update the train command and verification block: remove the
dangling line-continuation backslash after the -l yaib_logs/ flag in the uv run
icu-benchmarks train command and change the echo error message that currently
references "../yaib_logs/mimic_demo" to reference "yaib_logs/mimic_demo" (or
match the CHECK path used by the if statement) so the log-path in the error
output matches the actual check; locate the train invocation (uv run
icu-benchmarks train and its -l yaib_logs/ flag) and the verification echo that
prints the missing directory to apply these fixes.
In `@docs/adding_model/instructions.md`:
- Around line 19-30: Several fenced code blocks lack language identifiers;
update each block containing shell invocations like the "icu-benchmarks train
..." commands to use ```bash and the block with "Hyperparameters for TFT model."
to use ```gin (or other appropriate language) so markdownlint MD040 is satisfied
and rendered output improves; search for the unlabeled fences around the
"icu-benchmarks train" examples and the "Hyperparameters for TFT model." section
and add the correct triple-backtick language tags.
In `@icu_benchmarks/models/train.py`:
- Around line 107-109: The code sets dataset_names = {} when omitted but later
does direct lookups dataset_names["train"], dataset_names["val"], and
dataset_names["test"], which can raise KeyError; update the accesses in train.py
to use safe retrieval (e.g., dataset_names.get("train", <fallback>), or
pre-populate dataset_names with default keys) wherever
dataset_names["train"]/["val"]/["test"] are used (references: dataset_names
variable and the direct indexing occurrences dataset_names["train"],
dataset_names["val"], dataset_names["test"]) so the function works when
dataset_names is not provided.
In `@icu_benchmarks/tuning/hyperparameters.py`:
- Around line 370-371: The log message is incorrect: inside the branch that
checks checkpoint.exists() you call logging.warning with "does not exist."
Change this to reflect the true state — replace the logging.warning call with
logging.info (or remove the warning) and update the message to something like
"Hyperparameter checkpoint {checkpoint} exists; loading checkpoint" so it
matches checkpoint.exists() and the subsequent "Loading checkpoint at
{checkpoint}" info log; edit the statement that references checkpoint.exists(),
logging.warning, and the subsequent "Loading checkpoint at {checkpoint}" log
accordingly.
---
Nitpick comments:
In `@icu_benchmarks/run_utils.py`:
- Around line 237-245: Remove the redundant unconditional concat/write block and
keep a single guarded operation that both checks shap_values_test and handles
errors: replace the two blocks with one try/except that first checks "if
shap_values_test:" then calls pl.concat(shap_values_test) and
shap_values.write_parquet(log_dir / "aggregated_shap_values.parquet"), and logs
any exception; reference the existing symbols shap_values_test, pl.concat,
write_parquet, and log_dir/"aggregated_shap_values.parquet" to locate and update
the duplicate code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 686b297a-da5a-48b5-8565-3810b2585ae3
⛔ Files ignored due to path filters (1)
docs/figures/yaib_flow_combined.svgis excluded by!**/*.svg
📒 Files selected for processing (96)
.github/workflows/ci.yml.github/workflows/code-quality.yml.github/workflows/e2e-integration.yml.github/workflows/python-build.yml.gitignore.pre-commit-config.yamlLICENSEPAPER.mdREADME.mdconfigs/imputation_models/BRNN.ginconfigs/imputation_models/Diffusion.ginconfigs/imputation_models/MICE.ginconfigs/imputation_models/NP.ginconfigs/imputation_models/RNN.ginconfigs/imputation_models/SSSDSA.ginconfigs/prediction_models/BRFClassifier.ginconfigs/prediction_models/CBClassifier.ginconfigs/prediction_models/GRU.ginconfigs/prediction_models/LSTM.ginconfigs/prediction_models/LogisticRegression.ginconfigs/prediction_models/RFClassifier.ginconfigs/prediction_models/RUSBClassifier.ginconfigs/prediction_models/Transformer.ginconfigs/prediction_models/XGBClassifier.ginconfigs/prediction_models/common/DLCommon.ginconfigs/prediction_models/common/DLTuning.ginconfigs/prediction_models/common/MLCommon.ginconfigs/prediction_models/common/MLTuning.ginconfigs/tasks/BinaryClassification.ginconfigs/tasks/BinaryClassificationNoDynamic.ginconfigs/tasks/BinaryClassificationNoStatic.ginconfigs/tasks/DatasetImputation.ginconfigs/tasks/Regression.ginconfigs/tasks/common/CrossValidation.ginconfigs/tasks/common/Dataloader.ginconfigs/tasks/common/Imports.ginconfigs/tasks/common/PredictionTaskVariables.gindocs/adding_model/instructions.mddocs/adding_model/rnn.pydocs/development.mddocs/imputation_methods.mddocs/wanddb.mdexperiments/benchmark_classification.ymlexperiments/benchmark_regression.ymlexperiments/demo_benchmark_classification.ymlexperiments/demo_benchmark_regression.ymlexperiments/experiment_eval_pooled.ymlexperiments/experiment_full_training.ymlexperiments/experiment_submission.shexperiments/slurm_base_features.shexperiments/slurm_base_template.shicu_benchmarks/cross_validation.pyicu_benchmarks/data/loader.pyicu_benchmarks/data/pooling.pyicu_benchmarks/data/preprocessor.pyicu_benchmarks/data/split_process_data.pyicu_benchmarks/imputation/amputations.pyicu_benchmarks/imputation/baselines.pyicu_benchmarks/imputation/csdi.pyicu_benchmarks/imputation/diffusion.pyicu_benchmarks/imputation/diffwave.pyicu_benchmarks/imputation/layers/s4layer.pyicu_benchmarks/imputation/mlp.pyicu_benchmarks/imputation/np.pyicu_benchmarks/imputation/rnn.pyicu_benchmarks/imputation/simple_diffusion.pyicu_benchmarks/imputation/sssds4.pyicu_benchmarks/imputation/sssdsa.pyicu_benchmarks/models/__init__.pyicu_benchmarks/models/constants.pyicu_benchmarks/models/custom_metrics.pyicu_benchmarks/models/dl_models/layers.pyicu_benchmarks/models/dl_models/rnn.pyicu_benchmarks/models/dl_models/tcn.pyicu_benchmarks/models/dl_models/transformer.pyicu_benchmarks/models/ml_models/imblearn.pyicu_benchmarks/models/ml_models/lgbm.pyicu_benchmarks/models/ml_models/sklearn.pyicu_benchmarks/models/ml_models/xgboost.pyicu_benchmarks/models/train.pyicu_benchmarks/models/utils.pyicu_benchmarks/models/wrappers.pyicu_benchmarks/run.pyicu_benchmarks/run_utils.pyicu_benchmarks/tuning/gin_utils.pyicu_benchmarks/tuning/hyperparameters.pyicu_benchmarks/wandb_utils.pypyproject.tomlscripts/evaluate_results/aggregrate_experiment.pyscripts/plotting/plotting.pyscripts/sample_usage/random_search/LGBM.shscripts/sample_usage/random_search/LSTM.shscripts/sweep_configs/01_experiment_1.yamlscripts/sweep_configs/hyperparameter_sweeps/brnn_sweep.ymlscripts/sweep_configs/hyperparameter_sweeps/np_sweep.ymlscripts/sweep_configs/hyperparameter_sweeps/saits_sweep.yml
💤 Files with no reviewable changes (14)
- configs/tasks/DatasetImputation.gin
- LICENSE
- configs/tasks/Regression.gin
- configs/prediction_models/Transformer.gin
- configs/tasks/BinaryClassificationNoStatic.gin
- configs/tasks/BinaryClassificationNoDynamic.gin
- configs/prediction_models/LogisticRegression.gin
- .github/workflows/ci.yml
- configs/tasks/BinaryClassification.gin
- configs/prediction_models/LSTM.gin
- configs/prediction_models/RFClassifier.gin
- configs/prediction_models/BRFClassifier.gin
- configs/prediction_models/GRU.gin
- configs/prediction_models/RUSBClassifier.gin
✅ Files skipped from review due to trivial changes (54)
- configs/tasks/common/CrossValidation.gin
- configs/prediction_models/common/DLTuning.gin
- configs/imputation_models/BRNN.gin
- experiments/demo_benchmark_regression.yml
- configs/imputation_models/NP.gin
- experiments/demo_benchmark_classification.yml
- configs/tasks/common/Imports.gin
- icu_benchmarks/models/init.py
- configs/imputation_models/SSSDSA.gin
- scripts/sweep_configs/01_experiment_1.yaml
- configs/imputation_models/MICE.gin
- configs/prediction_models/common/MLCommon.gin
- docs/wanddb.md
- icu_benchmarks/models/ml_models/imblearn.py
- experiments/experiment_full_training.yml
- icu_benchmarks/models/ml_models/sklearn.py
- configs/tasks/common/PredictionTaskVariables.gin
- icu_benchmarks/wandb_utils.py
- configs/imputation_models/RNN.gin
- configs/imputation_models/Diffusion.gin
- scripts/sweep_configs/hyperparameter_sweeps/brnn_sweep.yml
- scripts/sample_usage/random_search/LGBM.sh
- docs/development.md
- icu_benchmarks/models/constants.py
- configs/prediction_models/CBClassifier.gin
- icu_benchmarks/models/dl_models/rnn.py
- icu_benchmarks/imputation/np.py
- scripts/sample_usage/random_search/LSTM.sh
- icu_benchmarks/tuning/gin_utils.py
- icu_benchmarks/imputation/mlp.py
- icu_benchmarks/models/ml_models/lgbm.py
- icu_benchmarks/models/ml_models/xgboost.py
- configs/prediction_models/common/MLTuning.gin
- experiments/slurm_base_features.sh
- icu_benchmarks/models/dl_models/transformer.py
- configs/prediction_models/XGBClassifier.gin
- icu_benchmarks/models/dl_models/tcn.py
- icu_benchmarks/cross_validation.py
- icu_benchmarks/data/pooling.py
- scripts/sweep_configs/hyperparameter_sweeps/saits_sweep.yml
- experiments/slurm_base_template.sh
- icu_benchmarks/imputation/diffusion.py
- experiments/benchmark_classification.yml
- icu_benchmarks/imputation/rnn.py
- scripts/sweep_configs/hyperparameter_sweeps/np_sweep.yml
- icu_benchmarks/imputation/baselines.py
- icu_benchmarks/imputation/simple_diffusion.py
- README.md
- icu_benchmarks/run.py
- docs/imputation_methods.md
- icu_benchmarks/models/utils.py
- icu_benchmarks/models/dl_models/layers.py
- icu_benchmarks/models/custom_metrics.py
- icu_benchmarks/imputation/csdi.py
🚧 Files skipped from review as they are similar to previous changes (1)
- PAPER.md
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
6-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSync the package version with
icu_benchmarks.__version__.
pyproject.tomlpublishes0.3.1, buticu_benchmarks/__init__.py:5still exports0.1.0. That leaves the wheel metadata and the runtime version API disagreeing for anyone who readsicu_benchmarks.__version__.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 6 - 7, pyproject.toml lists version "0.3.1" but icu_benchmarks.__version__ (in icu_benchmarks/__init__.py) still exports "0.1.0", causing runtime and wheel metadata to disagree; fix by synchronizing them — either update the literal in icu_benchmarks/__init__.py to "0.3.1" or change __version__ to derive from package metadata (e.g., importlib.metadata.version) so the value always matches the pyproject.toml version; make sure the unique symbol icu_benchmarks.__version__ reflects the published version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pyproject.toml`:
- Around line 6-7: pyproject.toml lists version "0.3.1" but
icu_benchmarks.__version__ (in icu_benchmarks/__init__.py) still exports
"0.1.0", causing runtime and wheel metadata to disagree; fix by synchronizing
them — either update the literal in icu_benchmarks/__init__.py to "0.3.1" or
change __version__ to derive from package metadata (e.g.,
importlib.metadata.version) so the value always matches the pyproject.toml
version; make sure the unique symbol icu_benchmarks.__version__ reflects the
published version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 89fae533-dceb-4830-b000-de8f4f28c35d
📒 Files selected for processing (2)
.github/workflows/python-build.ymlpyproject.toml
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.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
1 participant
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.
Summary by CodeRabbit
Chores
New Features
Documentation
Refactor