Feature/update #6

denis-yuen · 2022-08-08T21:11:13Z

switch to github packages for the Docker image to help empower team to update image (using regular github permissions)
switch build from travis ci to github actions to enable workflow run testing again
update some dependency versions to get this working
integrates Pull changes from base repo #2
should make Update README.md #4 redundant

https://ucsc-cgl.atlassian.net/browse/DOCK-2179

TODO: can also add dockstore cli testing, but I think this is getting out of scope, so will create a ticket on merge
TODO: will also need to tag and replace this branch name with a tag after merge

denis-yuen · 2022-08-08T21:11:31Z

~~TODO: cherry-pick relevant stuff from other two PRs~~

denis-yuen · 2022-08-09T15:08:17Z

~~TODO: add .dockstore.yml and test that things work with apptool CLI~~

…bamstats into feature/update

This reverts commit b5515dd.

…bamstats into feature/update

aofarrel

The naming scheme of the workflows should be improved. I've heard that nextflow files generally follow a particular naming schema but we can improve "Dockstore2.cwl, "Dockstore.wdl", and "Dockstore.cwl". I would recommend calling them bamstats (or in the case of Dockstore2.cwl, bamstats_sort). We should encourage users to give their workflows meaningful names.

denis-yuen · 2022-08-11T17:28:35Z

these are now https://dockstore.org/containers/github.com/dockstore/dockstore-tool-bamstats/bamstats_sort_cwl:feature/update?tab=info and https://dockstore.org/containers/github.com/dockstore/dockstore-tool-bamstats/bamstats_cwl:feature/update?tab=info

coverbeck · 2022-08-11T17:45:00Z

install.sh

@@ -6,11 +6,11 @@ set -o nounset
 set -o xtrace

 if [[ "${LANGUAGE}" == "cwl" ]]; then
-    curl -o requirements.txt "https://raw.githubusercontent.com/dockstore/dockstore/1.7.0-rc.3/dockstore-webservice/src/main/resources/requirements/1.7.0/requirements3.txt"
+    curl -o requirements.txt "https://raw.githubusercontent.com/dockstore/dockstore/1.13.0-beta.3/dockstore-webservice/src/main/resources/requirements/1.13.0/requirements3.txt"
    pip3 install --user -r requirements.txt
 elif [[ "${LANGUAGE}" == "wdl" ]]; then
    wget https://github.com/broadinstitute/cromwell/releases/download/44/cromwell-44.jar


Do we need this? We don't have instructions on how to run directly with Cromwell, and the CLI is going to download its own version of Cromwell, not use this one.

Actually, I only looked at the diff, so perhaps we do reference it from unmodified code. Maybe update to the same version the CLI uses if so though.

I think I'd probably clean this up in can also add dockstore cli testing, but I think this is getting out of scope, so will create a ticket on merge

(i.e. we can test with cromwell for now just to make sure it is valid WDL and works, then when I switch to dockstore CLI, it will automatically be running the "right" version)

aofarrel

This needs to be tested before being merged if hasn't already been; if I'm not mistaken the WDL will not work at all with the current setup due to Cromwell seemingly not supporting ghcr.io according to my testing: broadinstitute/cromwell#6827

Note that Cromwell sometimes claims to have an error with a Docker image, and then proceeds to use it without an issue, and/or fumbles creds when pulling images, so there is a chance that I'm the fool here. All I know is that Docker can pull such images just fine but Cromwell throws an error when I stick them into a WDL.

aofarrel · 2022-08-13T00:00:19Z

.dockstore.yml

+    primaryDescriptorPath: /bamstats.wdl
+    testParameterFiles:
+      - /test.wdl.json
+    name: wdl


workflow name should be "bamstats_wdl" or something like that

Seems redundant. The name of the tool is already github.com/dockstore/dockstore-tool-bamstats/wdl

I think most people view the last part as the de facto name of the workflow, even though strictly speaking that's not true, and if it's redundancy we're worried about I think "dockstore-tool" is the bigger issue.

Well, the last part is optional as well. It just distinguishes between tools in the same repo.

I agree that the second dockstore is kinda redundant too, but two redundant(s) don't make a non-redundant? 😉

There's also the inconsistency -- we put bamstats in the names of the CWLs. Why not do the same with the WDL and NFL?

(Note that this is a nitpick at the moment and I'm not willing to die on this hill, but depending if/how we address dockstore/dockstore#4790 and dockstore/dockstore#4491 this could have implications later.)

aofarrel · 2022-08-13T00:00:34Z

.dockstore.yml

+      - orcid: 0000-0002-6130-1021
+  - subclass: NFL
+    primaryDescriptorPath: /nextflow.config
+    name: nfl


this also needs to be more specific

Ditto with Seems redundant. The name of the tool is already github.com/dockstore/dockstore-tool-bamstats/wdl

aofarrel · 2022-08-13T00:03:35Z

README.md

 manually you would execute:

-    docker build -t collaboratory/dockstore-tool-bamstats:1.25-7 .
+    docker build -t ghcr.io/dockstore/dockstore-tool-bamstats:1.25-8 .


I recently discovered that if you host the image on ghcr.io, this will trigger an error in Cromwell (and by extension the Dockstore CLI -- can confirm I get the error with this URL in the latest stable version of the Dockstore CLI).

broadinstitute/cromwell#6827

See github action, error seems like more of a warn/warning that we can ignore

[2022-08-11 17:24:51,73] [warn] BackendPreparationActor_for_e2a7a05d:bamstatsWorkflow.bamstats:-1:1 [e2a7a05d]: Docker lookup failed java.lang.Exception: Registry ghcr.io is not supported

I believe it's not acceptable to present to the user with an unclear warning on a tutorial repository. We should really consider moving this to Docker Hub.

README.md

aofarrel · 2022-08-13T00:09:55Z

bamstats.wdl

@@ -15,7 +15,7 @@ task bamstats {
 	}

 	runtime {
-		docker: "quay.io/collaboratory/dockstore-tool-bamstats:1.25-7"
+		docker: "ghcr.io/dockstore/dockstore-tool-bamstats:1.25-8"


Please make sure this WDL has been confirmed to actually work as it is written here, because in my testing this registry doesn't work with Cromwell/the Dockstore CLI. If you've already confirmed it works, please also let me know because then that means the issues I have with ghcr.io are my own fault.

In fact, it's worth manually confirming every workflow in this repo works as expected with the calls we're giving, if that hasn't already been done. Most of the issues I had with this repo previously were things not working as expected.

My bad, I misinterpreted what the GH Action is doing the first time around but I think I get it now. I still stand by what I said about not throwing warnings at new users but the most important thing is that things work.

denis-yuen · 2022-08-15T14:52:42Z

This needs to be tested before being merged if hasn't already been; if I'm not mistaken the WDL will not work at all with the current setup due to Cromwell seemingly not supporting ghcr.io according to my testing: broadinstitute/cromwell#6827

Note that Cromwell sometimes claims to have an error with a Docker image, and then proceeds to use it without an issue

This PR is adding the github action testing is part of this PR already
https://github.com/dockstore/dockstore-tool-bamstats/actions/runs/2841613321
This behaviour is confirmed, it claims to not be able to support github packages but runs it successfully anyway since the image is already built (and probably works if you download the image first).

aofarrel

Cromwell's warning about where the image is hosted is confusing to new users and will likely lead them to conclude that the registry is the issue if anything goes wrong (ie they typo the command or are playing with the repo post-tutorial). This is on the Cromwell dev team to fix, but we can sidestep the issue entirely by hosting on Docker Hub. Docker Hub has its own issues but given that this is a tutorial repo, Cromwell's bogus warnings have a lot more weight than they would for a standard tool/workflow.

denis-yuen · 2022-08-15T17:32:50Z

Cromwell's warning about where the image is hosted is confusing to new users and will likely lead them to conclude that the registry is the issue if anything goes wrong (ie they typo the command or are playing with the repo post-tutorial). This is on the Cromwell dev team to fix, but we can sidestep the issue entirely by hosting on Docker Hub. Docker Hub has its own issues but given that this is a tutorial repo, Cromwell's bogus warnings have a lot more weight than they would for a standard tool/workflow.

I get where you're coming from and all things being equal, I'd probably agree.

Sadly, over the year's we've lost access to https://hub.docker.com/u/dockstore
We've tried the "recover account" approach to no avail. We could try a new Docker Hub org, but the underlying issue (that we have too many accounts to manage/juggle) is part of why I'd like to use Github packages more (to simplify our accounts and empower more developers on our team to work with our Docker images).

Since the warning is just a warning and doesn't stop execution ... I'm a little tempted to fix it on the cromwell side. BRB

aofarrel · 2022-08-15T18:58:20Z

@denis-yuen If you are looking into fixing this on the Cromwell side, here's what I know:

If the Docker image is present on the machine already, the warning is still thrown but see my recent comment for a caveat
- In other words: It is possible that my Cromwell-6828 ticket has it backwards and that Cromwell can pull from local images but will throw the registry warning anyway.
- If that is the case then it is possible bamstats will not work for new users, as my understanding is that our tests create the image locally first thereby obscuring issues users may (or may not!) have pulling the image. In terms of bamstats what we need to be certain of is that, if someone is running the workflow with the image not on the machine, Cromwell can pull that successfully. That's what I'm currently unsure of (and unfortunately lack the bandwidth to test in a timely manner).
Could be related to java.lang.Exception: Unauthorized to get docker hash broadinstitute/cromwell#6674, which seems to happen with quay.io images (I could've sworn quay images didn't use to do this!)
I dislike Cromwell being overly verbose so I looked into logging levels and discovered that Cromwell uses akka and has a concept of info-warning-error in its logging, but there doesn't seem to be a simple way to switch between those logging levels. If you are trying to suppress warnings in Cromwell then my understanding is that you'll need to do some hacks on the level of akka.

denis-yuen · 2022-08-15T19:55:53Z

dislike Cromwell being overly verbose so I looked into logging levels and discovered that Cromwell uses akka and has a concept of info-warning-error in its logging, but there doesn't seem to be a simple way to switch between those logging levels.

Actually, this works java -DLOG_LEVEL=ERROR -jar ... documented here https://cromwell.readthedocs.io/en/stable/Logging/

denis-yuen · 2022-08-15T20:02:27Z

After poking around in the Cromwell codebase, discovered good news and bad news.

This error message has nothing to do with actually pulling the image. Rather it has to do with calculating a Docker hash, much the way we do for checksums. Presumably, it is used to verify the exact Docker image used but this means it actually doesn't affect how it runs the workflow.
This is why cromwell just continues past the warning (which isn't an error) and this is why II was able to verify Terra is just fine running this version of the workflow as well.
So yes, 6828 likely has it backwards

It wasn't trivial to add gchr.io support to get these Docker hashes

denis-yuen · 2022-08-15T20:06:35Z

(i.e. I'll probably just suppress it when I do the CLI follow-up)

denis-yuen · 2022-08-16T15:19:18Z

Conversation to be continued in following PR

aofarrel · 2022-08-16T17:02:49Z

dislike Cromwell being overly verbose so I looked into logging levels and discovered that Cromwell uses akka and has a concept of info-warning-error in its logging, but there doesn't seem to be a simple way to switch between those logging levels.

Actually, this works java -DLOG_LEVEL=ERROR -jar ... documented here https://cromwell.readthedocs.io/en/stable/Logging/

WHAT

denis-yuen added 12 commits August 8, 2022 14:39

replace Docker image

d809aa6

Create workflows.yml

77408b0

mv

262c9f4

attempt variable hookup

ef46525

syntax

7dcad81

this?

7b339ac

install dependencies?

27d3ef5

paths?

a0936cb

sigh

8aaa555

more syntax

b572b04

right, cwltool doesn't like 3.10

3b185c0

update

fa95ade

denis-yuen self-assigned this Aug 8, 2022

Merge branch 'develop' into feature/update

b9208fe

denis-yuen added 9 commits August 9, 2022 11:10

try this

b5515dd

Merge branch 'feature/update' of github.com:dockstore/dockstore-tool-…

85619e9

…bamstats into feature/update

Revert "try this"

33bfac1

This reverts commit b5515dd.

how about this cwl?

1ebfa5a

what if even newer requirements

47145f6

Create .dockstore.yml

d155c5a

Update .dockstore.yml

73ddf08

Update .dockstore.yml

22618c0

Update README.md

8109144

denis-yuen requested review from aofarrel, a team, david4096 and kathy-t and removed request for a team August 9, 2022 17:42

Merge branch 'feature/update' of github.com:dockstore/dockstore-tool-…

771b044

…bamstats into feature/update

david4096 approved these changes Aug 10, 2022

View reviewed changes

svonworl approved these changes Aug 10, 2022

View reviewed changes

kathy-t approved these changes Aug 11, 2022

View reviewed changes

denis-yuen requested review from aofarrel and coverbeck and removed request for aofarrel August 11, 2022 14:16

aofarrel suggested changes Aug 11, 2022

View reviewed changes

rename stuff

279cb67

denis-yuen requested a review from aofarrel August 11, 2022 17:28

coverbeck approved these changes Aug 11, 2022

View reviewed changes

unito-bot mentioned this pull request Aug 11, 2022

Migrate Dockstore created/maintained Tools to using GitHub app dockstore/dockstore#3831

Closed

aofarrel suggested changes Aug 13, 2022

View reviewed changes

Add note on cromwell [skip ci]

cb3105f

consistency [skip ci]

35a09b0

denis-yuen requested a review from aofarrel August 15, 2022 14:59

aofarrel suggested changes Aug 15, 2022

View reviewed changes

denis-yuen requested a review from aofarrel August 15, 2022 20:06

denis-yuen mentioned this pull request Aug 16, 2022

mimic readme cli steps with github actions #7

Merged

denis-yuen merged commit 82afe11 into develop Aug 16, 2022

denis-yuen deleted the feature/update branch August 16, 2022 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/update #6

Feature/update #6

denis-yuen commented Aug 8, 2022 •

edited

Loading

denis-yuen commented Aug 8, 2022 •

edited

Loading

denis-yuen commented Aug 9, 2022 •

edited

Loading

aofarrel left a comment

denis-yuen commented Aug 11, 2022

coverbeck Aug 11, 2022

coverbeck Aug 11, 2022

denis-yuen Aug 11, 2022

denis-yuen Aug 11, 2022

aofarrel left a comment •

edited

Loading

aofarrel Aug 13, 2022

denis-yuen Aug 15, 2022

aofarrel Aug 15, 2022

denis-yuen Aug 15, 2022

aofarrel Aug 15, 2022

aofarrel Aug 13, 2022

denis-yuen Aug 15, 2022 •

edited

Loading

aofarrel Aug 13, 2022

denis-yuen Aug 15, 2022

aofarrel Aug 15, 2022

aofarrel Aug 13, 2022

aofarrel Aug 13, 2022

denis-yuen Aug 15, 2022

aofarrel Aug 15, 2022

denis-yuen commented Aug 15, 2022 •

edited

Loading

aofarrel left a comment

denis-yuen commented Aug 15, 2022

aofarrel commented Aug 15, 2022

denis-yuen commented Aug 15, 2022

denis-yuen commented Aug 15, 2022

denis-yuen commented Aug 15, 2022

denis-yuen commented Aug 16, 2022

aofarrel commented Aug 16, 2022

Feature/update #6

Feature/update #6

Conversation

denis-yuen commented Aug 8, 2022 • edited Loading

denis-yuen commented Aug 8, 2022 • edited Loading

denis-yuen commented Aug 9, 2022 • edited Loading

aofarrel left a comment

Choose a reason for hiding this comment

denis-yuen commented Aug 11, 2022

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

aofarrel left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-yuen Aug 15, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-yuen commented Aug 15, 2022 • edited Loading

aofarrel left a comment

Choose a reason for hiding this comment

denis-yuen commented Aug 15, 2022

aofarrel commented Aug 15, 2022

denis-yuen commented Aug 15, 2022

denis-yuen commented Aug 15, 2022

denis-yuen commented Aug 15, 2022

denis-yuen commented Aug 16, 2022

aofarrel commented Aug 16, 2022

denis-yuen commented Aug 8, 2022 •

edited

Loading

denis-yuen commented Aug 8, 2022 •

edited

Loading

denis-yuen commented Aug 9, 2022 •

edited

Loading

aofarrel left a comment •

edited

Loading

denis-yuen Aug 15, 2022 •

edited

Loading

denis-yuen commented Aug 15, 2022 •

edited

Loading