Add override mechanism for algorithm containers, transfer explicit .sif images with HTCondor

[jhiemstrawisc]

This PR aims to accomplish a few things:

1. It adds a new valid keys to config.yaml that let a user say "For reconstruction algorithm foo, I want you to use container image bar". In the general case, I'm not sure how much this is needed, but @agitter and I have floated the concept in the past, and it helps me solve 2. This implements a 4-tier hierarchy over how much of the image name is overridden (with special logic for .sif extensions), e.g.:

containers:
  registry:
    base_url: docker.io
    owner: reedcompbio
    
    images:
      omicsintegrator1: "oi1:latest"
      omicsintegrator2: "jhiemstra/oi2:latest
      mincostflow: "hub.opensciencegrid.org/jhiemstra/mincostflow:latest
      allpairs: "images/allpairs.sif"

would result in container URIs of

• omicsintegrator1 --> docker.io/reedcompbio/oi1:latest
• omicsintegrator2 --> docker.io/jhiemstra/oi2:latest1
• mincostflow --> hub.opensciencegrid.org/jhiemstra/mincostflow:latest
• allpairs --> images/allpairs.sif (local file)

The caveat here is that I don't believe all container registries follow this kind of <base_url>/<owner>/<container> hierarchy, but it's at least true for docker, ghcr and hub.opensciencegrid.org. If a user finds themselves somewhere outside those, they can always declare the entire URI explicitly.

2. When a container override contains the .sif extension, the file is added to the reconstruct rule's htcondor_transfer_input_files resource key. This triggers HTCondor to transfer the sif image as part of the job's input sandbox.
3. When a .sif override is present and apptainer/singularity is the configured container framework, SPRAS uses the local file instead of pulling/building from a remote registry. This is combined with 2) are the key fixes for Provide guidance on working around docker rate limiting for large CHTC runs #462
4. Some of the container resolution logic started to get sufficiently nested with if/else logic that I wanted to split it out into a helper function I could more easily test. This is where most of the new test code comes from.
5. Finally, I fixed a small-but-inconsequential bug I noticed in the way we treated singularity and apptainer container frameworks differently in some cases. I added a helper function that makes it easier to treat these as synonyms.

Note that I haven't yet documented this guidance, as #462 requests -- I'd rather get this over the finish line first, then add the documentation to #459 (so I don't create conflicts for myself)

[agitter]

These changes make sense to me overall.

[read-the-docs-community]

Documentation build overview

📚 spras | 🛠️ Build #32474638 | 📁 Comparing 2599ec8 against latest (87b314c)

  🔍 Preview build  

5 files changed · ± 5 modified

± Modified

• genindex.html
• contributing/index.html
• fordevs/modules.html
• fordevs/spras.config.html
• fordevs/spras.html

[tristan-f-r]

This seems fine from a big picture: I am of course wary of:

• The dynamic algorithm import for the new ALGORITHM_REGISTRY, but the dependency diamond there seems unavoidable.
• The strange conflation for having both image_override and images in the same structure, but I have no clean ideas for addressing that at the moment [and I don't want to block this PR on waiting for a better refactor to come along].

I'll add some smaller comments 👍

[agitter]

The updates addressed my major comments. I only have one minor new one.

However, my local tests fail. Some are unrelated to these changes, some are related:

FAILED test/RWR/test_RWR.py::TestRWR::test_rwr - AssertionError: Output file does not match expected output file
FAILED test/ResponseNet/test_rn.py::TestResponseNet::test_responsenet_required - AssertionError: assert False
FAILED test/ResponseNet/test_rn.py::TestResponseNet::test_responsenet_all_optional - AssertionError: assert False
FAILED test/ST_RWR/test_STRWR.py::TestSTRWR::test_strwr - AssertionError: Output file does not match expected output file
FAILED test/analysis/test_cytoscape.py::TestCytoscape::test_cytoscape - spras.containers.ContainerError: (Command formatted as list: `['python', '/py4cytoscape/cytoscape_util.py', '--output', '/spras/T32FPZG/cytoscape.cys', '--pathway', '/spras/5M63HVH/pathway.txt|test\\analysis\\...
FAILED test/analysis/test_summary.py::TestSummary::test_example_networks[example] - AssertionError: assert False
FAILED test/analysis/test_summary.py::TestSummary::test_example_networks[egfr] - AssertionError: assert False
FAILED test/evaluate/test_evaluate.py::TestEvaluate::test_node_precision_recall_pca_chosen_pathway - assert False
FAILED test/generate-inputs/test_generate_inputs.py::TestGenerateInputs::test_prepare_inputs_networks - AssertionError: assert False
FAILED test/parse-outputs/test_parse_outputs.py::TestParseOutputs::test_parse_outputs - AssertionError: assert False
FAILED test/parse-outputs/test_parse_outputs.py::TestParseOutputs::test_duplicate_edges - AssertionError: assert False
FAILED test/test_container_image_resolution.py::TestPrepareSingularityImage::test_local_sif_with_unpack_builds_sandbox - AttributeError: module 'spython' has no attribute 'main'
FAILED test/test_container_image_resolution.py::TestPrepareSingularityImage::test_registry_with_unpack_pulls_and_builds - AttributeError: module 'spython' has no attribute 'main'
FAILED test/test_container_image_resolution.py::TestPrepareSingularityImage::test_local_sif_no_unpack_returns_sif_path - ModuleNotFoundError: No module named 'pwd'
FAILED test/test_container_image_resolution.py::TestPrepareSingularityImage::test_no_override_no_unpack_returns_docker_uri - ModuleNotFoundError: No module named 'pwd'
FAILED test/test_container_image_resolution.py::TestPrepareSingularityImage::test_unpack_skips_build_if_sandbox_exists - AttributeError: module 'spython' has no attribute 'main'

@tristan-f-r do you want to leave more detailed comments still?

[agitter]

I suspect the root cause of my spython and pwd errors is that pwd is Unix only: https://docs.python.org/3/library/pwd.html. If that's true, we could skip those tests or mock pwd.

[tristan-f-r]

Sounds good. I didn't find anything in my code passthrough, unfortunately.

[tristan-f-r]

[After tinkering with #457,] there does seem to be a better solution to this that avoids the dynamic algorithm loading entirely:

We can localize images to specific algorithms, e.g.:

allpairs:
  image_override: "..."
  runs: ...

and we can consider this as a kind of "run setting." I'll see if I can lift this run setting configuration out of #457 for general usage.

[jhiemstrawisc]

[After tinkering with #457,] there does seem to be a better solution to this that avoids the dynamic algorithm loading entirely:

We can localize images to specific algorithms, e.g.:

allpairs:
  image_override: "..."
  runs: ...

Tony and I discussed this approach and ultimately decided against it because the chosen approach keeps container logic in the yaml's container configuration.

[tristan-f-r]

Should we have a way to override the Cytoscape docker image as well?

[tristan-f-r]

Small cytoscape note: I don't have an immediate way to address that problem without making cytoscape a psuedo-algorithm, so I'm wondering if we even need to worry about it.

[tristan-f-r]

Should we have a way to override the Cytoscape docker image as well?

In line with this, noting that resolved_container_image effectively (with one warning-based exception) ignores container_suffix when the respective image_override is specified, we can instead have it so that instead of each algorithm specifying their own hard-coded container_suffix that is overriden in containers.py, we can instead have each algorithm poll ProcessedContainerSettings directly. As a more concrete sketch of this with e.g. PathLinker, we can have the following:

(for this line in pathlinker.py:)

-container_suffix = "pathlinker:v2"
+container_suffix = container_settings.resolve_image("pathlinker")

where then, in config.yaml, containers.registry.images would in-code have all of the defaults set to the appropriate container versions, which we can do quite easily with pydantic. For the example above, containers.registry.images.pathlinker would have the default value pathlinker:v2, and we would use your existing "container string -> resolved image" pipeline for ProcessedContainerSettings#resolve_image.

This means that we do have to maintain two separate lists of docker containers and algorithms, but then that also means we can differentiate between our list of algorithms versus other containers (e.g. Cytoscape), while getting free test-time guarantees that all algorithms are specified, since we explicitly would depend on the default values specified in such a container registry when calling resolve_image. (This also generalizes to, say, cases where algorithms have run several docker containers with different tooling: we already see behavior approaching this with DOMINOs usage of two resource-heavy tools, though they are luckily present in the same container with little difference in dependencies.)

[If you agree with this change but also don't want to implement it, I can try my hand at making a PR over your fork of SPRAS to make these changes.]

[agitter]

@tristan-f-r in your proposed redesign, what would the new config.yaml syntax look like? Would that change?

Where would the two lists of docker containers and algorithms need to be maintained? We would still have the ALGORITHM_REGISTRY in config.util and now we would also have a list of default images in containers.registry.images? I'm usually reluctant to have two similar lists to manually maintain because it creates opportunities for errors as new contributors add methods or things diverge over time.

It was a good catch that we can't override Cytoscape currently. Our existing Cytoscape support is weak, so if we exclude it in this pull request and document that, I am okay deferring a more general solution for a later time when someone needs that functionality.

[tristan-f-r]

config.yaml would look exactly the same: it's the internals I'm worried about. Specifically, we can drop the use of dynamic module loading in ALGORITHM_REGISTRY with this (instead continuing to use the old algorithms dictionary currently in spras), since the containers configuration would no longer require knowing algorithm names to inject image_override into their respective container settings.

Effectively, I would like to decouple container logic from the rest of SPRAS more than the currently implemented ALGORITHM_REGISTRY solution allows.