Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve or replace CWL md5sum repo #4886

Closed
aofarrel opened this issue Apr 29, 2022 · 5 comments
Closed

Improve or replace CWL md5sum repo #4886

aofarrel opened this issue Apr 29, 2022 · 5 comments
Assignees
Labels
cli documentation enhancement review merged but pending a third party look at whether it makes sense/is working
Milestone

Comments

@aofarrel
Copy link
Contributor

aofarrel commented Apr 29, 2022

Context

#4586 (comment) see that entire ticket for more details

Describe the bug

https://github.com/dockstore-testing/dockstore-workflow-md5sum-unified

  • Throws warnings in cwltool
  • Has a checker workflow which calls a workflow which itself calls a tool
  • Said checker workflow cannot be imported into SB as-is
  • Even after being modified in a way to allow SB import, said checker wf will fail in SB unless you edit with SB's rabix editor
  • Appears to be used in our integration tests ("dockstore-workflow-md5sum-unified" has 8 hits in the dockstore/dockstore repo)
  • Is used in our documentation (checksum-support.rst)
  • Is based on the various md5sum workflow/tools that are used all over our docs, so there is a chance those examples may also have issues ("md5sum" has 91 hits in dockstore/dockstore-documentation)

To Reproduce

Throws warnings in cwltool:

  1. Clone the repo locally
  2. cwltool checker_workflow_wrapping_workflow.cwl --input_file md5sum.json
  3. See warnings about foaf towards the top of the output

Has a checker workflow which calls a workflow which itself calls a tool:
By this I mean checker_workflow_wrapping_workflow.cwl, which calls md5sum/md5sum.cwl, which calls md5sum/dockstore-tool-md5sum.cwl. It appears that checker_workflow_wrapping_tool.cwl skips the middleman.

Cannot be imported into SB as-is:

  1. Go to https://dockstore.org/workflows/github.com/dockstore-testing/dockstore-workflow-md5sum-unified/cwl_checker:develop?tab=files
  2. Click button to import into SB via BDC
  3. Select a workspace to import into
  4. Get unhelpful error
    This one can be partially remedied by replacing the contents of md5sum/md5sum.cwl with md5sum/dockstore-tool-md5sum.cwl, which turns this checker workflow-->workflow-->tool import chain into checker workflow-->tool, but...

Even after being modified in a way to allow SB import, still fail in SB unless you edit them in SB's rabix editor
If you replace the contents of md5sum/md5sum.cwl with md5sum/dockstore-tool-md5sum.cwl like mentioned above, you will be able to import into SB, but when trying to actually run the workflow, you will get a "graph not connected" error.

Expected behavior

Throws warnings in cwltool:
The CWLs we use for testing and documentation should not throw warnings.

Has a checker workflow which calls a workflow which itself calls a tool:
This is fine in and of itself, and it does work in cwltool, but...

Cannot be imported into SB as-is
...currently this workflow-->workflow-->tool chain breaks if you try to import it into SB. Our examples should be able to import cleanly into SB, even if are not showing off the import into SB in said example, as it is a common thing to do.

Even after being modified in a way to allow SB import, still fail in SB unless you edit them in SB's rabix editor
This might be a bug with Rabix, but either way, our examples should work in SB without the user needing to debug them.

Why one big ticket?

  • Because these are all connected and provide context for each other
  • Because further investigation is needed to find out what is a TRS issue, a Dockstore issue, and/or a SB issue
  • Because the sum total of this ticket might mean that the short term answer is to replace some of the repos we use for documentation and testing, but maybe that's not necessary if it turns out there's a simple fix
  • Because I don't have a lot of experience with CWL and TRS, so there's a chance my guesses on how to tackle this/what's going on might be way off base 😬

If this isn't a quick issue, and assuming I'm not way off base with my guesses as to what's going on, I'd split it like this:

  1. investigate the app import failure
  2. investigate the node not connected issue
  3. replace at least this md5sum stuff from docs + tests
  4. investigate if the other md5sum stuff that dockstore-workflow-md5sum-unified is based upon are also problematic (warnings, errors, cannot be imported, node not connected)
  5. fix the warnings r/e foaf/namespaces

┆Issue is synchronized with this Jira Story
┆fixVersions: Dockstore 1.13
┆friendlyId: DOCK-2162
┆sprint: 93- Ursula
┆taskType: Story

@denis-yuen
Copy link
Member

Implementer could check if we've fallen out of date with https://www.commonwl.org/user_guide/17-metadata/index.html

@denis-yuen denis-yuen added the cli label May 2, 2022
@denis-yuen denis-yuen added this to the 1.13 milestone May 2, 2022
@denis-yuen
Copy link
Member

denis-yuen commented May 2, 2022

common-workflow-library/bio-cwl-tools#101 may point to a change from dct and foaf to schema.org

@denis-yuen
Copy link
Member

Breaking down ticket:

  1. Probably doesn't hurt to clean-up the dct schema, but it isn't invalid cwl. For example
$ cwltool  --validate checker_workflow_wrapping_workflow.cwl
INFO /home/dyuen/.local/bin/cwltool 3.1.20220224085855
INFO Resolved 'checker_workflow_wrapping_workflow.cwl' to 'file:///home/dyuen/dockstore-workflow-md5sum-unified/checker_workflow_wrapping_workflow.cwl'
URI prefix 'foaf' of 'foaf:name' not recognized, are you missing a $namespaces section?
URI prefix 'foaf' of 'foaf:mbox' not recognized, are you missing a $namespaces section?
URI prefix 'foaf' of 'foaf:name' not recognized, are you missing a $namespaces section?
URI prefix 'foaf' of 'foaf:mbox' not recognized, are you missing a $namespaces section?
URI prefix 'foaf' of 'foaf:name' not recognized, are you missing a $namespaces section?
URI prefix 'foaf' of 'foaf:mbox' not recognized, are you missing a $namespaces section?
URI prefix 'foaf' of 'foaf:name' not recognized, are you missing a $namespaces section?
URI prefix 'foaf' of 'foaf:mbox' not recognized, are you missing a $namespaces section?
checker_workflow_wrapping_workflow.cwl is valid CWL.
  1. Inability to import into SBG seems like a SBG bug

Suggest implementer fix warnings, re-attempt import into SBG, open* bug either way in an appropriate SBG repo with details about what type of CWL is unable to be imported and link to here.

  • or find existing ticket, add comments, vote

@denis-yuen
Copy link
Member

Above suggestions implemented to the workflow repo.
Remaining work is probably to create a tag and then update the docs as needed

@unito-bot unito-bot added the review merged but pending a third party look at whether it makes sense/is working label Aug 25, 2022
@unito-bot
Copy link

➤ David Steinberg commented:

Confirmed that running via cwltool was without issue.

dockstore-testing/dockstore-workflow-md5sum-unified@5ac641c ( dockstore-testing/dockstore-workflow-md5sum-unified@5ac641cc0e20997ee232f3273155f23f58383f29|smart-link )

Webservice ( https://github.com/dockstore/dockstore/releases/tag/1.13.0 ) - 1.13.0

UI ( https://github.com/dockstore/dockstore-ui2/releases/tag/2.10.0 ) - 2.10.0

Compose Setup ( https://github.com/dockstore/compose_setup/releases/tag/1.13.0 ) - 1.13.0

Deploy ( https://github.com/dockstore/dockstore-deploy/releases/tag/1.12.6 ) - 1.12.6

galaxyParsingLambdaVersion: 0.0.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli documentation enhancement review merged but pending a third party look at whether it makes sense/is working
Projects
None yet
Development

No branches or pull requests

4 participants