improved build workflow #272

bkmartinjr · 2023-03-17T21:50:09Z

This implements most of what is required to complete #265. Specifically, it dockerizes and automates the full census build steps 1-3:

verifies that the host meets requirements (memory, free disk, etc)
performs build & validation using cell_census_builder package, with official default args
generate summary and summary diff from latest build

It stops short of the remaining steps for a full release:

copies build & build logs to S3
performs release (release.json file edit) to latest
cleans up outdated releases (including removing from release.json)
re-eval notebooks

Changes in this PR:

Dockerized the Census builder
Add GHA to build the docker image
cell_census_builder converted to installable package, with sub-modules that perform different build steps. Each sub-module has a separate __main__ to allow for continued stand-alone use. Used a src layout for easy packaging with setuptools
- Added new top-level CensusBuildArgs, CensusBuildConfig and CensusBuildState which unifies the static and dynamic state across all builder sub-modules. This replaces the use of argparse.Namespace for configuration.
- Added a new top-level main that implements the official "census" build workflow, with defaults appropriate for full-census builds.
- Reorganized previous build code into a build_soma sub-module. This includes splitting __main__.py into multiple files, moving all files down into a sub-module called build_soma, etc.
- Added new sub-module host_validation which performs checks on host config/resources. Intended to be used as a prologue to a build, e.g., confirming sufficient memory
- Moved census_summary into the package as a sub-module
Moved scripts unrelated to build workflow (e.g., swapon) into tools/scripts as they will be used outside of the eventual container
revised tests around new package organization

Notes for reviewers: no major changes to the build or summary modules. Changes are primary around configuration and creating a new top-level workflow that has defaults suitable for the full Census build. There is also other cleanup and reorganization that could be done (e.g., breaking up the build_soma sub-module into smaller chunks), but I'd rather do that progressively, as this PR is getting way too big....

codecov · 2023-03-17T22:01:52Z

Codecov Report

Merging #272 (509ffd2) into main (91d7a12) will decrease coverage by 1.95%.
The diff coverage is 81.77%.

@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
- Coverage   93.44%   91.49%   -1.95%     
==========================================
  Files          34       41       +7     
  Lines        2028     2223     +195     
==========================================
+ Hits         1895     2034     +139     
- Misses        133      189      +56

Flag	Coverage Δ
unittests	`91.49% <81.77%> (-1.95%)`	⬇️

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

Impacted Files	Coverage Δ
...c/cell_census_builder/build_soma/census_summary.py	`100.00% <ø> (ø)`
...der/src/cell_census_builder/build_soma/datasets.py	`95.34% <ø> (ø)`
...lder/src/cell_census_builder/build_soma/globals.py	`100.00% <ø> (ø)`
...l_census_builder/build_soma/summary_cell_counts.py	`94.11% <ø> (ø)`
...rc/cell_census_builder/build_soma/tissue_mapper.py	`84.81% <ø> (ø)`
...builder/src/cell_census_builder/build_soma/util.py	`87.14% <ø> (ø)`
...census_builder/src/cell_census_builder/__init__.py	`60.00% <60.00%> (ø)`
...sus_builder/src/cell_census_builder/build_state.py	`60.78% <60.78%> (ø)`
...der/src/cell_census_builder/build_soma/__main__.py	`66.66% <66.66%> (ø)`
..._census_builder/src/cell_census_builder/logging.py	`73.07% <73.07%> (ø)`
... and 19 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

.github/workflows/py-build.yml

ebezzi · 2023-03-22T19:56:26Z

tools/cell_census_builder/src/cell_census_builder/build_soma/manifest.py

@@ -68,7 +68,7 @@ def load_manifest_from_CxG() -> List[Dataset]:
    logging.info(f"Found {len(datasets)} datasets, in {len(collections)} collections")

    # load per-dataset schema version
-    with concurrent.futures.ThreadPoolExecutor(max_workers=16) as tp:
+    with concurrent.futures.ThreadPoolExecutor(max_workers=32) as tp:


Is there a specific reason for which this isn't parametrized as well?

no good reason, other than we had no prior need. I'll add a config for it so it can be overridden. It can't use the standard worker config, as it controls the concurrent HTTP requests. The only reason we have any value here (rather than use the thread pool default) is to keep from overwhelming the Discover REST backend.

We could call it max_io_workers.

with the recent improvements to the Discover backend, this works fine using the system defaults for ThreadPoolExecutor so I simply removed the config entirely.

ebezzi · 2023-03-22T20:04:49Z

tools/cell_census_builder/README.md

+```
+working_dir:
+    |
+    +-- config.yaml        # build config (user provided, read-only)


Could be useful to provide an example config.yaml that can be copied as necessary.

sure. later info in this doc points the user at the build_state.py file. And by default you should not specify a config file if doing the standard census build. It is only for dev reasons that you would use it.

example added, plus verbiage about not needing to provide a config for the standard census build

rename to dev-config.yaml?

I would rather leave as-is. It is the config, for any purpose. I just have defaults set so that it isn't required in our current workflow, but I can easily imagine situations where it is used in the future.

that said, if you feel strongly, I'm not going to force this issue :-)

atolopko-czi

LGTM, but made a few usability/doc-related comments, plus:

The docker build failed on M1 ("numpy not found"), so attempted on EC2 Ubuntu instance, which worked. (Did not troubleshoot M1, since I don't think it's necessary to be able to build locally.)
On EC2, needed to install docker by following https://docs.docker.com/engine/install/ubuntu/#installation-methods and https://docs.docker.com/engine/install/linux-postinstall/#manage-docker-as-a-non-root-user. Worth noting in the README? Even though we intend to run this in Batch, I assume we'll be testing on EC2 on occasion.
Add build tools/cell_census_builder/requirements-dev.txt for times you are trying to build image locally (not via GHA).

atolopko-czi · 2023-03-23T13:18:34Z

tools/cell_census_builder/README.md

+```
+working_dir:
+    |
+    +-- config.yaml        # build config (user provided, read-only)


rename to dev-config.yaml?

atolopko-czi · 2023-03-23T13:20:32Z

tools/cell_census_builder/README.md

+```
+$ mkdir /tmp/census-build
+$ chmod ug+s /tmp/census-build   # optional, but makes permissions handling simpler
+$ docker run --mount type=bind,source="`pwd`/tmp/census-build",target='/census-build' cell-census-builder


Suggested change

$ docker run --mount type=bind,source="`pwd`/tmp/census-build",target='/census-build' cell-census-builder

$ docker run --mount type=bind,source="/tmp/census-build",target='/census-build' cell-census-builder

rm $ prompts for easier copy & paste; also add markdown type: "```shell" as first line (here & above)

atolopko-czi · 2023-03-23T13:27:11Z

tools/cell_census_builder/README.md


-https://docs.google.com/document/d/1GKndzCk9q_1SdYOq3BeCxWgp-o2NSQkEmSBaBPKnNI8/


Unless running from Batch is right around the corner, worth keeping this link or moving its contents here. E.g. you still need to know how to provision EC2 instance and setup swap.

this was a link to an obsolete schema spec, which I replaced with the correct link. It was not a build process doc, which has never been part of this README (and probably should not IMHO given that it contains internal infra info).

ps. once I get all this landed, and refined, I will update the "manual build process" doc, not linked here.

atolopko-czi · 2023-03-23T13:29:10Z

tools/cell_census_builder/README.md

+docker system prune
+docker rm -f $(docker ps -aq)
+docker rmi -f $(docker images -q)
+```


Move into a Makefile target for convenience?

excellent idea.

atolopko-czi · 2023-03-23T13:31:35Z

tools/cell_census_builder/src/cell_census_builder/__init__.py

+    from importlib import metadata
+except ImportError:
+    # for python <=3.7
+    import importlib_metadata as metadata  # type: ignore[no-redef]


nit: support for <=3.7 doesn't seem necessary

and the pyproject.toml defines supported releases as 3.9 and 3.10, so this change is consistent with that.

bkmartinjr · 2023-03-23T15:48:51Z

The docker build failed on M1 ("numpy not found"), so attempted on EC2 Ubuntu instance, which worked. (Did not troubleshoot M1, since I don't think it's necessary to be able to build locally.)

@atolopko-czi - can you provide more details on this failure? While I agree we do not support M1 Macs, this is an unexpected failure as numpy is clearly listed in the pyproject.toml dependencies. I would have expected that it would blow up trying to find an M1 wheel for tiledbsoma, which AFAIK does not yet exist.

atolopko-czi

LGTM!

Will recreate the numpy error later and report back.

atolopko-czi · 2023-03-23T16:19:58Z

tools/cell_census_builder/README.md


-https://docs.google.com/document/d/1GKndzCk9q_1SdYOq3BeCxWgp-o2NSQkEmSBaBPKnNI8/


bkmartinjr added 5 commits March 17, 2023 20:56

reorganize census builder

10b30cd

Merge branch 'main' into bkmartinjr/265-build-workflow

fedb07f

refactor files in build_soma

2399c97

fix GHA unit test

c30d38b

fix GHA unit test

d3cb272

bkmartinjr added 23 commits March 20, 2023 19:04

additional refactoring for top-level workflow

613cc46

Merge branch 'main' into bkmartinjr/265-build-workflow

376e852

add missing package to dependency list

4ae07c1

cleanup host validation config

a8963ee

update test CLI for host validation

76c6bbd

more namespace refactoring

bccd4f1

add reports to workflow

6dcb088

lint

e8829e0

handle default config correctly

2c2dba9

fix typo in defaults

4680c87

fix report typo

cb0d3f7

fix state load issue; enable multi-process by default

d307616

fix typo in program name

1f70ab9

add build resumption

e40653a

dockerfile update

18e1b67

docker build refinement

c35554a

refine builder build process

fe93cb3

add GHA for docker image build

5da72ec

update readme

accb53b

Merge branch 'main' into bkmartinjr/265-build-workflow

b9a2606

fix entry point

40b1c90

more readme edits

86993ee

fix owlready2 installation in docker image

2c8e2b0

bkmartinjr marked this pull request as ready for review March 22, 2023 19:33

bkmartinjr requested review from ebezzi and atolopko-czi March 22, 2023 19:34

ebezzi approved these changes Mar 22, 2023

View reviewed changes

bkmartinjr added 4 commits March 22, 2023 20:45

PR feedback

f103b5c

PR feedback

756d6f7

fix email address in metadata

1510369

Merge branch 'main' into bkmartinjr/265-build-workflow

74bd3bf

bkmartinjr added the sprint-March13-March24 label Mar 22, 2023

bkmartinjr added 5 commits March 23, 2023 01:14

add file size integrity check on downloads

0566b17

Merge branch 'main' into bkmartinjr/265-build-workflow

4618fb1

add missing broken process pool logger

84acafb

tweak developer Makefile for builder

4f50e11

clean up comments

e4f4c57

atolopko-czi requested changes Mar 23, 2023

View reviewed changes

PR feedback

2eb588e

bkmartinjr requested a review from atolopko-czi March 23, 2023 16:17

fix typo

509ffd2

atolopko-czi approved these changes Mar 23, 2023

View reviewed changes

bkmartinjr merged commit d9bd1eb into main Mar 23, 2023

bkmartinjr deleted the bkmartinjr/265-build-workflow branch March 23, 2023 16:56

bkmartinjr mentioned this pull request Mar 24, 2023

create top-level containerized script for full census build #265

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improved build workflow #272

improved build workflow #272

bkmartinjr commented Mar 17, 2023 •

edited

Loading

codecov bot commented Mar 17, 2023 •

edited

Loading

ebezzi Mar 22, 2023

bkmartinjr Mar 22, 2023 •

edited

Loading

ebezzi Mar 22, 2023

bkmartinjr Mar 22, 2023 •

edited

Loading

ebezzi Mar 22, 2023

bkmartinjr Mar 22, 2023

bkmartinjr Mar 22, 2023

atolopko-czi Mar 23, 2023

bkmartinjr Mar 23, 2023 •

edited

Loading

atolopko-czi left a comment

atolopko-czi Mar 23, 2023

atolopko-czi Mar 23, 2023

atolopko-czi Mar 23, 2023

atolopko-czi Mar 23, 2023

bkmartinjr Mar 23, 2023

bkmartinjr Mar 23, 2023

atolopko-czi Mar 23, 2023

atolopko-czi Mar 23, 2023

bkmartinjr Mar 23, 2023

atolopko-czi Mar 23, 2023

bkmartinjr Mar 23, 2023

bkmartinjr commented Mar 23, 2023

atolopko-czi left a comment

atolopko-czi Mar 23, 2023

	$ docker run --mount type=bind,source="`pwd`/tmp/census-build",target='/census-build' cell-census-builder
	$ docker run --mount type=bind,source="/tmp/census-build",target='/census-build' cell-census-builder


		https://docs.google.com/document/d/1GKndzCk9q_1SdYOq3BeCxWgp-o2NSQkEmSBaBPKnNI8/

improved build workflow #272

improved build workflow #272

Conversation

bkmartinjr commented Mar 17, 2023 • edited Loading

codecov bot commented Mar 17, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

bkmartinjr Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkmartinjr Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkmartinjr Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

atolopko-czi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkmartinjr commented Mar 23, 2023

atolopko-czi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkmartinjr commented Mar 17, 2023 •

edited

Loading

codecov bot commented Mar 17, 2023 •

edited

Loading

bkmartinjr Mar 22, 2023 •

edited

Loading

bkmartinjr Mar 22, 2023 •

edited

Loading

bkmartinjr Mar 23, 2023 •

edited

Loading