Convert cggnn/graphs and workflow configure to use config files (redo)

[CarlinLiao]

Re-open of #271. Closes #272 and #278.

Testing this will require changing a Docker image that #276/#281 and soon main will need to pass testing, so I've held off on making that change. Alternatively, we could make it so that the cggnn nextflow file doesn't refer to the spt-cg-gnn:latest but rather a specific version like spt-cg-gnn:test-1?

[jimmymathews]

I see a couple of merge conflicts after updates to main. Can you take a look? Probably you know what to do with them.

[CarlinLiao]

Conflicts resolved (but it's still not ready to merge yet).

[CarlinLiao]

All tests pass on my machine, this is ready to merge. Philosophically, spt-plugin#3 should be merged first because this change uses a Docker image in cggnn.nf that incorporates the updated plugin system, but it isn't required.

[CarlinLiao]

This will now also close #284. Originally, I was going to make a separate PR for that, but it required a lot of back and forth between the Docker repos and SPT so I just wrapped it up into this PR.

[jimmymathews]

As noted here, I am observing "24% overlap" of important cells, failing the cggnn-dockerized test. Should we update the reference cell set, or is this potentially evidence of something wrong? Are you also getting this behavior?

I am also getting failure to rebuild the data-loaded images, due to the API-breaking changes to spt workflow ... commands. I can fix this up I think. Since it is only a handful of arguments that are needed to keep the same usage (study name, db config file), I may revert the implementation to accept these as CLI arguments so that a separate workflow config file is not required. After all, this was the original reason for having the command spt workflow configure; to create a config file from a single command line.

[CarlinLiao]

I didn't observe either of these errors when I tested with my local machine. It was after a fresh clean-docker-images too.

[jimmymathews]

make force-rebuild-data-loaded-images should have created errors for you, since the scripts that import test data have not been updated with `--config-file=... arguments.

[CarlinLiao]

I see, I didn't rebuild my data loaded images specifically. I thought that clean-docker-images would do that but I imagine if it did it would take even longer.

[jimmymathews]

The data-preloaded images are now rebuilding correctly. All test pass for me (except the graph one with insufficient reproducibility). I reviewed the code changes and I think I understand most of it.

[CarlinLiao]

Ok, we have a problem in that I don't have a problem.

I ran clean-docker-images and force-rebuild-data-loaded-images before running module-test-workflow and got the following result:

┃ cggnn-dockerized ┈┈┈N E X T F L O W  ~  version 23.10.0
Launching `./main.nf` [extravagant_easley] DSL2 - revision: 8080837e53
executor >  local (7)
[11/57cfb6] process > echo_environment_variables [100%] 1 of 1 ✔
[b8/8e6022] process > prep_graph_creation        [100%] 1 of 1 ✔
[b3/d44f37] process > create_specimen_graphs (1) [100%] 2 of 2 ✔
[e2/326ffe] process > finalize_graphs            [100%] 1 of 1 ✔
[52/0ed50c] process > train                      [100%] 1 of 1 ✔
[74/de4c37] process > upload_importance_scores   [100%] 1 of 1 ✔

Output model size is within 1% of expected size.
Output importance scores file is the expected length 149.
Got expected approximate average .39 .
66% of the most important histological structures match the reference.
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ Passed.        (38s) 

This was reproducible in that I cleaned my Docker images and rebuilt my data loaded images again to get a virtually identical result.

┃ cggnn-dockerized ┈┈┈N E X T F L O W  ~  version 23.10.0
Launching `./main.nf` [happy_venter] DSL2 - revision: 8080837e53
executor >  local (7)
[03/c6f4a0] process > echo_environment_variables [100%] 1 of 1 ✔
[cc/dda21d] process > prep_graph_creation        [100%] 1 of 1 ✔
[50/d5ec9d] process > create_specimen_graphs (2) [100%] 2 of 2 ✔
[8b/56291f] process > finalize_graphs            [100%] 1 of 1 ✔
[74/1aad2b] process > train                      [100%] 1 of 1 ✔
[c9/698e9c] process > upload_importance_scores   [100%] 1 of 1 ✔
Completed at: 10-Jan-2024 20:09:18
Duration    : 1m 15s
CPU hours   : (a few seconds)
Succeeded   : 7

Output model size is within 1% of expected size.
Output importance scores file is the expected length 149.
Got expected approximate average .39 .
66% of the most important histological structures match the reference.
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ Passed.        (88s)  

I've verified that I'm using the spt-cg-gnn image from DockerHub so I have no idea what the issue could be.

I wonder if it's a mix of hardware differences running the deep learning pipeline and random chance of which cells are identified as "important". There are only 100 cells being used to create graphs so 66% on my machine vs 24% may not be too farfetched. It might be a good sign that the values are reproducible on the same machine?

[jimmymathews]

Hm why don't you just commit a new reference, probably achieving 100% for you and hopefully >50% for me. It may be too much to expect reproducibility on such small unstructured datasets like this test data.

[jimmymathews]

And yes it is a good sign that the same results are obtained, it seems, in the same environment.

[CarlinLiao]

I've updated the reference importances. Notably, there are fewer cells in the new importances than the old ones, meaning that fewer cells were captured in the graphs made. I think this reflects a change made in the graph generation strategy we made last month. I vaguely recall tweaking the implementation a bit in response to a request, but I don't remember what it was.

[CarlinLiao]

Looking back, I don't see any change to the graph generation process in the last PR where you started to see that difference in important cells. The only part that was changed was moving of the graph creation moving out to the external Docker image. The change in environment is the most likely culprit.

[CarlinLiao]

Oh, I see you mean "break" as in the workflow API has changed, not "break" as in the workflow CLI API no longer works, as that was something we fixed yesterday.

[CarlinLiao]

I'll update the nadeemlab/spt-cg-gnn:latest image after this commit is merged with the same image as 0.0.2. Going forward I assume we'll want to continually update :latest but peg the version the SPT workflow uses to a specific image version like 0.0.2.

[jimmymathews]

All tests pass, with 83% overlap in that one test. This is substantially concordant.