Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[misc] acceptance test GHA workflow #897

Merged
merged 42 commits into from
Dec 20, 2023
Merged

Conversation

bkmartinjr
Copy link
Contributor

@bkmartinjr bkmartinjr commented Dec 16, 2023

Add GHA which peforms the R and Python "acceptance" tests on a schedule - currently every Saturday at 1AM UTC. Can also be run manually via workflow_dispatch.

By default, will install from main and run the latest acceptance tests in main. You can run it against a branch with:

gh workflow run full-unittests.yml --ref branch_name

The python (not R) job supports installing a custom tiledbsoma version, allowing the main of this repo to test against any tiledbsoma version, including branches from that repo. This will allow pre-qualification of tiledbsoma releases against the latest cellxgene-census code base. For example, to test against the head of main, do:

gh workflow run full-unittests.yml -f 'tiledbsoma_python_dependency=git+https://github.com/single-cell-data/TileDB-SOMA.git#egg=tiledbsoma&subdirectory=apis/python/'

Other changes:

  • update to R acceptance test to reduce memory use
  • minor fix to version dep on setuptools for python package
  • minor fix to Python tests to work with latest LTS Census release

Current test run status:

  • Python - works
  • R - generates warnings about long vectors (32 bit limitation of R)

Overhang:

Copy link

codecov bot commented Dec 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7cca895) 86.81% compared to head (a2f19a6) 86.81%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #897   +/-   ##
=======================================
  Coverage   86.81%   86.81%           
=======================================
  Files          72       72           
  Lines        5255     5255           
=======================================
  Hits         4562     4562           
  Misses        693      693           
Flag Coverage Δ
unittests 86.81% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@bkmartinjr bkmartinjr changed the title full unit test workflow for python (big runner) [misc] acceptance test GHA workflow Dec 17, 2023
@@ -1,5 +1,5 @@
[build-system]
requires = ["setuptools>=45", "setuptools_scm[toml]>=6.2"]
requires = ["setuptools>=64", "setuptools_scm[toml]>=8"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only tangentially related to the new GHA, but important as the existing dependencies were quite obsolete and willl fail in some scenarios. See https://setuptools-scm.readthedocs.io/en/latest/

@bkmartinjr bkmartinjr marked this pull request as ready for review December 17, 2023 21:35
Copy link
Collaborator

@atolopko-czi atolopko-czi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, w/some very minor suggestions.

.github/workflows/full-unittests.yml Outdated Show resolved Hide resolved
.github/workflows/full-unittests.yml Outdated Show resolved Hide resolved
.github/workflows/full-unittests.yml Outdated Show resolved Hide resolved
.github/workflows/full-unittests.yml Outdated Show resolved Hide resolved
@bkmartinjr bkmartinjr marked this pull request as draft December 18, 2023 16:55

r_unit_tests:
if: false # TEMPORARILY DISABLE FOR DEBUGGING
runs-on: single-cell-1tb-runner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woot! just wanted to say 👋🏻 and introduce myself. I worked with @beroy getting these self-hosted runners set up. I'm watching them periodically and recording metrics on the nodes and autoscaler, but ping me if you see any unusual behavior or have other requirements you want us to add to them. Have fun!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @jakeyheath - these are a game changer. Really happy to have them!

So far they seem to work great, other than missing some "nice-to-have" packages installed in their default image. I have given @beroy an initial list.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy to solve, we already chatted this morning about building a custom container for this project to use that has all the packages pre-installed.

Let's meet in the new year and see how things are going. Right now, everything is running in our czi-playground account. Might be good to put this project is a more specific location and tune the performance as needed. Here's a screenshot of the metrics I collected on the latest run. Happy to show yall how to pull these yourselves and tune to your needs.

Screenshot 2023-12-18 at 12 15 17 PM

@@ -209,7 +209,7 @@ def test_hvg_vs_scanpy(
"Homo sapiens",
"is_primary_data == True",
"dataset_id",
slice(500_000, 1_000_000),
slice(1_000_000, 4_000_000),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the latest LTS census has no primary cells in the [500_000,1_000_000] soma joniid range, so this test will fail. Expanded the range to ensure we pick up some cells.

@@ -102,7 +102,7 @@ test_that("test_incremental_read_X_human", {
})

test_that("test_incremental_read_X_human-large-buffer-size", {
census <- open_soma_latest_for_test(soma.init_buffer_bytes = paste(4 * 1024**3))
census <- open_soma_latest_for_test(soma.init_buffer_bytes = paste(1 * 1024**3))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on very high cpu count machines, this will OOM due to per-core buffer allocation. Reducing to a GiB resolves the issue

@bkmartinjr bkmartinjr marked this pull request as ready for review December 18, 2023 21:49
Copy link
Member

@ebezzi ebezzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's ship those now and monitor how they go.

@bkmartinjr bkmartinjr merged commit 2f2bfd7 into main Dec 20, 2023
17 checks passed
@bkmartinjr bkmartinjr deleted the bkmartinjr/full-unit-test branch December 20, 2023 16:42
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.

None yet

4 participants