perf: remove per-minibatch overhead from the data path and the inference forward - #4017
Open
JESUSROYETH wants to merge 1 commit into
Open
JESUSROYETH wants to merge 1 commit into
JESUSROYETH wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4017 +/- ##
=======================================
Coverage 89.46% 89.47%
=======================================
Files 247 247
Lines 24389 24393 +4
=======================================
+ Hits 21820 21826 +6
+ Misses 2569 2567 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Collaborator
|
Thank you @JESUSROYETH ! I will validate further with other tutorials, but generally I think it safe to have this addition towards next version. |
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.
Four places paid Python work once per minibatch (or once per call) that only needs to happen once, or not at all. Nothing changes in the results: the ELBO of the introduction tutorial is the same float on both sides, and the sha256 of
get_latent_representation,get_normalized_expression,get_likelihood_parameters, thedifferential_expressionDataFrame and a minibatch fromAnnTorchDatasetare identical (19 of 19).Changes
AnnTorchDataset.data/__getitem__.ilocon every minibatch: 129 µs of a 214 µs batch fetchdatais first materialised: 3 µsauto_move_data,BaseModuleClass.device{p.device for p in self.parameters()}on every decorated call (3 per inference minibatch), 22 µs each on the default SCVInext(self.parameters()).device, 1 µsscrna_raw_counts_propertiesmultiply()returns COO andCOO.mean(axis=0)sorts every entry first: 100 ms per DE group.tocsr()before the mean: 13 ms, bit-identicalget_likelihood_parametersnp.concatenate(dropout_list)inside the minibatch loop, quadratic in cells (18 641 cells: 1.19 s)The device set only existed to raise if a module was split across devices, and no module in scvi-tools does that (Lightning moves the
pl_moduleas a whole); a real split still fails with PyTorch's own device-mismatch error, just later.Measurements
Introduction tutorial as published (
heart_cell_atlas_subsampled, 18 641 × 1 200,cell_source+donorandpercent_mito+percent_riboas covariates,train_size=0.8,check_val_every_n_epoch=1), base and patched run alternately in fresh processes, median of 5 runs, delta as median (min…max) over the runs:get_latent_representation()get_normalized_expression()get_likelihood_parameters()differential_expression(), one pairdifferential_expression(mode="change"), 11 groupsThe training gain needs registered covariates; the other rows apply to every model that shares the decorated forward and the DE core.
Validation
New tests:
test_covariates_materialized_as_numpy,tests/module/test_decorators.py,tests/module/test_base_module.py(the DataFrame and no-parameter cases fail on currentmain) andtest_scrna_raw_counts_properties_matches_dense_reference(all six statistics against a dense NumPy reference).tests/data,tests/dataloaders,tests/module,tests/model/test_scvi.py,test_totalvi.py,test_scanvi.py,test_peakvi.py,test_differential.py,test_pyro.pyand theexternal/models that read.devicepass on CPU;ruff checkandruff format --checkare clean. CHANGELOG entries added under 1.5.2, I will add the PR number once it exists.