search

nadeemlab / SPT

Spatial profiling toolbox for spatial characterization of tumor immune microenvironment in multiplex images
https://oncopathtk.org
Other
21 stars 2 forks source link

Move plugin repos into this main repo #325

Closed CarlinLiao closed 5 months ago

CarlinLiao commented 5 months ago

To close #323. Adds SPT graph plugins and includes them in the docker build and push Makefile recipes. I've verified that make build-docker-images recipe does not error and did a dry run make -n build-and-push-docker-images to verify that it's assembling the recipe as expected, but I haven't run it because I'm afraid of overwriting the images in Docker Hub, especially the plugin images that're in active use.

I looked into it and discovered that the submodule feature was added to git in v1.5.3, which released in 2007.

CarlinLiao commented 5 months ago

I had some consternation when it came to naming directories. The plugins' source code is in plugin/ but the Dockerfiles are in build/plugins/. This makes sense to me since plugin/ contains both all the plugins' source code as well as template(s) that are not themselves a plugin, while build/plugins/ contains all the Dockerfiles for the actual plugins that're meant to be built, but I can see how it can be confusing seeing as I've already confused myself while writing the Makefile recipes.

Similarly, I added the graph-generation subdirectory to both plugin/ and build/plugins/ under the assumption that at some point in the feature we want to support other plugin archetypes, even though this made writing the Makefile recipes marginally more complicated. (I even made an unpushed commit flattening the structure before reversing it.) However, I've structured the Docker Hub repos and Makefile recipes in a way such that it's assumed that the plugin names cannot clash with submodules or plugin names from other plugin archetypes, since graph-generation isn't included in the name of the Docker Hub repo but every repo is prepended with /nadeemlab/spt-.

CarlinLiao commented 5 months ago

Added a quick hot fix for a mostly unrelated issue that unnecessarily prepended "cuda-" to the version number for plugins that require CUDA to begin with (i.e., graph-transformer). I'm surprised this wasn't caught earlier given that we've been running the graph-transformer workflow to test it before, which worries me I'm doing something wrong this time.

CarlinLiao commented 5 months ago

Through testing I discovered that the graph-transformer:0.0.1 Docker image was bugged, so I've uploaded a new one using the new Makefile recipe to fix it.

The errors were disqualifying (it lacked necessary apt packages to run cv2 and was not updated to switch to relative references when the script was moved from app/ to /usr/bin/local broke references), so I'm puzzled how we were able to run and upload graph-transformer results beforehand.