chanzuckerberg / cellxgene-census

CZ CELLxGENE Discover Census
https://chanzuckerberg.github.io/cellxgene-census/
MIT License
72 stars 19 forks source link

[python] share census objects in tests for faster test time #1156

Closed ivirshup closed 1 month ago

ivirshup commented 1 month ago

I propose we re-use SOMA objects in testing to cut down on total test time.

From a single measurement, I see a little more than a 10% (~40 seconds) decrease in test time with this change. I've run the get_anndata tests on this commit and the one before it. I haven't run this many times, so I don't know what the variance is. You can see that with this commit there is far lower time taken on setup for each task (since the resource is simply removed)

Timings on this commit

``` ================================================================================ slowest durations ================================================================================ 53.89s call tests/test_get_anndata.py::test_get_anndata_allows_missing_obs_or_var_filter 34.26s call tests/test_get_anndata.py::test_get_anndata_value_filter 33.00s call tests/test_get_anndata.py::test_get_anndata_obs_embeddings[obs_embeddings0] 31.75s call tests/test_get_anndata.py::test_get_anndata_two_layers[layers0] 30.18s call tests/test_get_anndata.py::test_get_anndata_two_layers[layers1] 16.68s call tests/test_get_anndata.py::test_get_anndata_var_embeddings[var_embeddings0] 13.84s call tests/test_get_anndata.py::test_get_anndata_x_layer[raw] 13.23s call tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[geneformer] 12.33s call tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[scvi] 7.79s call tests/test_get_anndata.py::test_get_anndata_x_layer[normalized] 7.64s call tests/test_get_anndata.py::test_get_anndata_obsm_two_layers[obsm_layers0] 6.23s call tests/test_get_anndata.py::test_get_anndata_coords 6.18s setup tests/test_get_anndata.py::test_get_anndata_value_filter 1.29s setup tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[scvi] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_obsm_layers_and_add_obs_embedding_fails 0.00s call tests/test_get_anndata.py::test_get_anndata_obsm_layers_and_add_obs_embedding_fails 0.00s setup tests/test_get_anndata.py::test_get_anndata_obsm_layers_and_add_obs_embedding_fails 0.00s teardown tests/test_get_anndata.py::test_get_anndata_var_embeddings[var_embeddings0] 0.00s call tests/test_get_anndata.py::test_get_anndata_wrong_layer_names 0.00s setup tests/test_get_anndata.py::test_get_anndata_x_layer[normalized] 0.00s setup tests/test_get_anndata.py::test_get_anndata_x_layer[raw] 0.00s setup tests/test_get_anndata.py::test_get_anndata_two_layers[layers1] 0.00s setup tests/test_get_anndata.py::test_get_anndata_two_layers[layers0] 0.00s setup tests/test_get_anndata.py::test_get_anndata_obsm_two_layers[obsm_layers0] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_obs_embeddings[obs_embeddings0] 0.00s setup tests/test_get_anndata.py::test_get_anndata_obs_embeddings[obs_embeddings0] 0.00s setup tests/test_get_anndata.py::test_get_anndata_var_embeddings[var_embeddings0] 0.00s setup tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[geneformer] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[geneformer] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_two_layers[layers0] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_x_layer[normalized] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_x_layer[raw] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_obsm_two_layers[obsm_layers0] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_two_layers[layers1] 0.00s setup tests/test_get_anndata.py::test_get_anndata_coords 0.00s teardown tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[scvi] 0.00s setup tests/test_get_anndata.py::test_get_anndata_wrong_layer_names 0.00s teardown tests/test_get_anndata.py::test_get_anndata_value_filter 0.00s teardown tests/test_get_anndata.py::test_get_anndata_allows_missing_obs_or_var_filter 0.00s setup tests/test_get_anndata.py::test_get_anndata_allows_missing_obs_or_var_filter 0.00s teardown tests/test_get_anndata.py::test_get_anndata_coords 0.00s teardown tests/test_get_anndata.py::test_get_anndata_wrong_layer_names =================================================================== 14 passed, 34 warnings in 268.35s (0:04:28) =================================================================== ```

Timings on main

``` ================================================================================ slowest durations ================================================================================ 44.75s call tests/test_get_anndata.py::test_get_anndata_allows_missing_obs_or_var_filter 35.88s call tests/test_get_anndata.py::test_get_anndata_obs_embeddings[obs_embeddings0] 34.50s call tests/test_get_anndata.py::test_get_anndata_two_layers[layers1] 33.52s call tests/test_get_anndata.py::test_get_anndata_two_layers[layers0] 28.05s call tests/test_get_anndata.py::test_get_anndata_value_filter 19.20s call tests/test_get_anndata.py::test_get_anndata_var_embeddings[var_embeddings0] 15.72s call tests/test_get_anndata.py::test_get_anndata_obsm_two_layers[obsm_layers0] 13.25s call tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[scvi] 12.79s call tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[geneformer] 11.51s call tests/test_get_anndata.py::test_get_anndata_x_layer[raw] 11.35s call tests/test_get_anndata.py::test_get_anndata_x_layer[normalized] 9.00s call tests/test_get_anndata.py::test_get_anndata_coords 6.48s setup tests/test_get_anndata.py::test_get_anndata_value_filter 5.26s setup tests/test_get_anndata.py::test_get_anndata_coords 5.18s setup tests/test_get_anndata.py::test_get_anndata_two_layers[layers0] 5.18s setup tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[geneformer] 3.15s setup tests/test_get_anndata.py::test_get_anndata_x_layer[raw] 3.06s setup tests/test_get_anndata.py::test_get_anndata_two_layers[layers1] 2.71s call tests/test_get_anndata.py::test_get_anndata_wrong_layer_names 1.38s setup tests/test_get_anndata.py::test_get_anndata_x_layer[normalized] 1.31s setup tests/test_get_anndata.py::test_get_anndata_allows_missing_obs_or_var_filter 1.29s setup tests/test_get_anndata.py::test_get_anndata_obsm_two_layers[obsm_layers0] 1.19s setup tests/test_get_anndata.py::test_get_anndata_obsm_layers_and_add_obs_embedding_fails 1.15s setup tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[scvi] 1.14s setup tests/test_get_anndata.py::test_get_anndata_obs_embeddings[obs_embeddings0] 1.11s setup tests/test_get_anndata.py::test_get_anndata_wrong_layer_names 1.08s setup tests/test_get_anndata.py::test_get_anndata_var_embeddings[var_embeddings0] 0.65s call tests/test_get_anndata.py::test_get_anndata_obsm_layers_and_add_obs_embedding_fails 0.00s teardown tests/test_get_anndata.py::test_get_anndata_var_embeddings[var_embeddings0] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_wrong_layer_names 0.00s teardown tests/test_get_anndata.py::test_get_anndata_obsm_layers_and_add_obs_embedding_fails 0.00s teardown tests/test_get_anndata.py::test_get_anndata_obs_embeddings[obs_embeddings0] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_obsm_two_layers[obsm_layers0] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[geneformer] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_value_filter 0.00s teardown tests/test_get_anndata.py::test_get_anndata_x_layer[raw] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_x_layer[normalized] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_two_layers[layers0] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_two_layers[layers1] 0.00s teardown tests/test_get_anndata.py::test_get_anndata_coords 0.00s teardown tests/test_get_anndata.py::test_get_anndata_allows_missing_obs_or_var_filter 0.00s teardown tests/test_get_anndata.py::test_get_anndata_obsm_one_layer[scvi] =================================================================== 14 passed, 34 warnings in 310.89s (0:05:10) =================================================================== ```
codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 96.22642% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 91.12%. Comparing base (10db44f) to head (e06dca6).

Files Patch % Lines
api/python/cellxgene_census/tests/conftest.py 75.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1156 +/- ## ========================================== - Coverage 91.16% 91.12% -0.04% ========================================== Files 77 77 Lines 5896 5892 -4 ========================================== - Hits 5375 5369 -6 - Misses 521 523 +2 ``` | [Flag](https://app.codecov.io/gh/chanzuckerberg/cellxgene-census/pull/1156/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=chanzuckerberg) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/chanzuckerberg/cellxgene-census/pull/1156/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=chanzuckerberg) | `91.12% <96.22%> (-0.04%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=chanzuckerberg#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ivirshup commented 1 month ago

For this kind of change, would you prefer that it's done incrementally or in one big PR?

prathapsridharan commented 1 month ago

If all you intend to do is change the scope of the census pytest.fixture from the default of function to session, then I think it is reasonable to make that change in a single PR in all the tests. But I would also like @ebezzi opinion here too

ivirshup commented 1 month ago

Cool. So the two other sets of tests where I think this can be used is test_lts_compat and test_get_helpers.py. I would think we should also share the fixture between the tests, so can move it to the conftest.