Skip to content

perf: cache loaded feature matrices between splits and models - #467

Open
jfrog64 wants to merge 2 commits into
daisybio:developmentfrom
jfrog64:perf-feature-cache
Open

perf: cache loaded feature matrices between splits and models#467
jfrog64 wants to merge 2 commits into
daisybio:developmentfrom
jfrog64:perf-feature-cache

Conversation

@jfrog64

@jfrog64 jfrog64 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

PR Checklist for all PRs

  • This comment contains a description of changes (with reason)
  • Referenced issue is linked — no existing issue, this came out of profiling a long single-drug run
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated — runyourmodel.rst documents the cache and the opt-out for model authors

Changes

Bug fixes

New features

The pipeline reloads the same feature matrices over and over. load_features loads them once per split,
train_and_predict loads them again whenever it is called without prepared features, and train_final_model
loads them once more. For single drug models the model is re-instantiated per drug and per split, so the
identical cell line matrix is read from disk for every one of them.

What that costs, measured on a CTRPv2 run with 545 drugs and 893 genes (ElasticNet, single drug mode,
5 splits): 17.8 s per load, 3270 loads, ~16 h of an 18.4 h run — 88 % of the wall clock was feature I/O,
and the expression matrix was re-read 6× per drug. After caching, the marginal cost per drug drops to ~3.3 s
and the projected runtime for the same experiment is ~2.5 h.

_load_features_cached therefore keeps loaded matrices in a process-level dict, keyed by everything they
depend on: model class name, feature kind, data path, dataset name, and the model's hyperparameters
(json.dumps(..., sort_keys=True), so the key does not depend on dict order). The hyperparameters are part of
the key because they decide which views and which gene list a model loads; build_model always runs before
the loaders, at all three call sites.

Why handing out the same object is safe. The consumers in experiment.py only ever read .identifiers
and .view_names from the returned dataset and pass .copy() to models, randomize_features and
cross_study_prediction. FeatureDataset.copy() is a deep copy (copy.deepcopy of features and
meta_info), so nothing a model does can reach the cached object. The three in-place mutators in the codebase
either copy first (prepare_expression_and_methylation, prepare_proteomics) or only ever see a copy
(DIPK's cell_line_input.apply). I verified this end to end on a real experiment before proposing it: over
1019 cell line views, the cached object was byte-identical to a fresh load (max |Δ| = 0), both directly after
loading and after a complete 2-drug, 5-split run including the final model; the number of real loads went from
12 to 0.

One class of model cannot use it, and that is now explicit. SparseGOModel.load_cell_line_features does
not only return features, it also builds the ontology structure (layer_connections, gene2id_mapping_ont,
ontology_gene_order, gene_dim_input) that train and predict need. Since model = model_class() creates
a fresh instance per split, serving the second split's features from the cache would skip that initialization
and train would raise "layer_connections or gene2id_mapping_ont are not set". DRPModel therefore gets a
class attribute supports_feature_caching (default True); SparseGO sets it to False and its loaders run
for every instance, exactly as before this PR. The cleaner long-term fix is to move the ontology construction
out of the loader, but that is a SparseGO refactor and does not belong in this PR.

Bounds and escape hatches.

  • The cache holds at most _FEATURE_CACHE_MAXSIZE = 4 entries and evicts in insertion order (FIFO, not LRU —
    the access pattern is "same key many times in a row", so recency reordering buys nothing). The bound is on
    the number of entries, not on bytes, so up to four full matrices can be resident.
  • clear_feature_cache() is public for callers that want the memory back between datasets; the pipeline itself
    never calls it.
  • DREVAL_FEATURE_CACHE=0 disables it completely, which is how one can A/B check that results are unchanged.
  • The cache is per process and is not shared across workers.

Tests: tests/test_feature_cache.py

  • repeated loads return the same object and call the loader once
  • a fresh model instance reuses the cached matrix — the case that matters for single drug models
  • the key separates feature kind, dataset, data path, hyperparameters and model class
  • hyperparameters in a different order describe the same matrix and share the entry
  • the cache evicts at _FEATURE_CACHE_MAXSIZE and the evicted dataset is loaded again
  • DREVAL_FEATURE_CACHE=0 restores the previous load-every-time behaviour and leaves the cache empty
  • load_features, the entry point the pipeline uses, goes through the cache
  • a model with supports_feature_caching = False loads for every instance, its loader's state is set on each
    of them, and nothing is cached
  • a guard that walks MODEL_FACTORY and asserts that every model whose feature loaders assign to self has
    opted out — so the next stateful loader cannot silently break

Maintenance

jfrog64 and others added 2 commits August 26, 2026 16:50
Feature matrices were reloaded from disk for every split, every
hyperparameter setting and, for single drug models, for every drug.
On a 545-drug CTRPv2 run this dominated the wall clock: 3270 loads at
roughly 17.8 s each, about 16 h of an 18.4 h run.

The matrices only depend on the model class, the feature kind, the data
path, the dataset name and the hyperparameters (which decide the views
and the gene list), so they are cached under exactly that key. The cache
holds at most four entries, since an entry is a full feature matrix, and
can be disabled with DREVAL_FEATURE_CACHE=0 to A/B check that results
are unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… state

SparseGO's load_cell_line_features does not only return features, it also
builds the ontology structure (layer_connections, gene2id_mapping_ont,
ontology_gene_order, gene_dim_input) that train() and predict() need. The
cache skips the loader for every instance after the first, so a fresh
instance per CV split would have been left half built and train() would
have raised "layer_connections or gene2id_mapping_ont are not set".

DRPModel now carries supports_feature_caching (default True); SparseGO
sets it to False and its features are loaded per instance again. A test
walks MODEL_FACTORY and asserts that every model whose feature loaders
assign to self has opted out, so the next such model cannot slip through.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 96.75325% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.22%. Comparing base (7d24cd6) to head (aaa1669).
⚠️ Report is 75 commits behind head on development.

Files with missing lines Patch % Lines
tests/test_feature_cache.py 96.06% 5 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #467      +/-   ##
===============================================
+ Coverage        80.34%   82.22%   +1.87%     
===============================================
  Files              101      121      +20     
  Lines             8171     9927    +1756     
===============================================
+ Hits              6565     8162    +1597     
- Misses            1606     1765     +159     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants