search

broadinstitute / palantir-workflows

Utility workflows for the DSP hydro.gen team (formerly palantir)
BSD 3-Clause "New" or "Revised" License
19 stars 8 forks source link

Hotfix: Update IGV xml writing docker to get benchmark tests passing #172

Closed kachulis closed 5 months ago

kachulis commented 5 months ago

IGV xml writing currently uses a very old docker image hosted on Maddie's personal quay.io account. This has recently led to test failures due to the deprecation of old docker image manifest schemes and (presumably) an upgrade in the version of docker run on cicle-ci machines (see https://docs.docker.com/engine/deprecated/#pushing-and-pulling-with-image-manifest-v2-schema-1).

I could not find a dockerfile for this docker anywhere, or even a copy of the writeIGV.sh script. The references I could find all pointed to a google bucket that doesn't seem to exist anymore. So I copied the script out of the old docker, and added into this repo, and created a new docker file and image for it. I pushed the image to us-central1-docker.pkg.dev/broad-dsde-methods/hydro-gen-dockers/igv-xml:v1.0 so we will have better control of it moving forward.

rickymagner commented 5 months ago

Have you checked if this doesn't already apply for your use case? It's a workflow/task you can import to create an IGV session xml with the code already included there, so there's no mystery dockers/scripts. I noticed our old one had this same problem so wrote that utility a few months ago.

rickymagner commented 5 months ago

The corresponding docs are here. I'd suggest maybe modifying that if you want further functionality rather than introducing something new.

rickymagner commented 5 months ago

(Also, updating the BenchmarkVCFs docker to replace this task with the new utility is a part of the benchmarking PR)

kachulis commented 5 months ago

oh fantastic! I looked at BenchmarkVCFs/CreateIGVSession.wdl and saw it was still on the old docker, but didn't realize there was a different version. Will switch to using that. And yeah, I just want a quick fix to get tests passing again, this is all moot once #167 is merged.

kachulis commented 5 months ago

ahh, actually looks like it isn't quite a drop-in like I thought, there's an extra renaming of tracks that happens in the current version. So I'm gonna leave this new docker hack in place for now just to get tests passing again. Feel free to completely replace with the new version in the future.

rickymagner commented 5 months ago

Ah, in that case, it sounds like the order of operations should be:

  1. Approve/merge this PR, just to make some tests pass for now.
  2. Update the benchmarking PR #167 to rebase off main and remove the stuff added here (so we don't have multiple IGV docker stuff floating around).
  3. Approve/merge the benchmarking PR.

I'll relabel this PR so that when we look back (if ever) it'll be more clear this was a temporary hotfix and NOT meant to be the source of longterm IGV session-making code.