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

WIP Fix resolver for imports #1093

Merged
merged 11 commits into from
Apr 1, 2019
Merged

WIP Fix resolver for imports #1093

merged 11 commits into from
Apr 1, 2019

Conversation

denis-yuen
Copy link
Member

  • resolve imports properly for TRS

test with commands like the following for command-line tools (previously working)

$ cat test.json
{
  "input_file": {
        "class": "File",
        "path": "md5sum.input"
    }
}
(venv) $ cwltool --make-template quay.io/briandoconnor/dockstore-tool-md5sum:1.0.4 test.json
/home/dyuen/cwltool/venv/bin/cwltool 1.0.20190323055837
Resolved 'quay.io/briandoconnor/dockstore-tool-md5sum:1.0.4' to 'https://dockstore.org/api/api/ga4gh/v2/tools/quay.io%2Fbriandoconnor%2Fdockstore-tool-md5sum/versions/1.0.4/plain-CWL/descriptor/Dockstore.cwl'
input_file:  # type "File"
    class: File
    path: a/file/path
(venv) $ cwltool quay.io/briandoconnor/dockstore-tool-md5sum:1.0.4 test.json
/home/dyuen/cwltool/venv/bin/cwltool 1.0.20190323055837
Resolved 'quay.io/briandoconnor/dockstore-tool-md5sum:1.0.4' to 'https://dockstore.org/api/api/ga4gh/v2/tools/quay.io%2Fbriandoconnor%2Fdockstore-tool-md5sum/versions/1.0.4/plain-CWL/descriptor/Dockstore.cwl'
[job Dockstore.cwl] Skipping Docker software container '--memory' limit despite presence of ResourceRequirement with ramMin and/or ramMax setting. Consider running with --strict-memory-limit for increased portability assurance.
... (snip)
Final process status is success

and for workflows (what was broken) since workflows have multiple files (e.g. https://dockstore.org/api/api/ga4gh/v2/tools/%23workflow%2Fgithub.com%2Fdockstore-testing%252Fmd5sum-checker/versions/develop/CWL/files )

$ cwltool --make-template \#workflow/github.com/dockstore-testing/md5sum-checker:develop
/home/dyuen/cwltool/venv/bin/cwltool 1.0.20190323055837
Resolved '#workflow/github.com/dockstore-testing/md5sum-checker:develop' to 'https://dockstore.org/api/api/ga4gh/v2/tools/%23workflow%2Fgithub.com%2Fdockstore-testing%2Fmd5sum-checker/versions/develop/plain-CWL/descriptor/md5sum-workflow.cwl'
input_file:  # type "File"
    class: File
    path: a/file/path
(venv) $ cwltool \#workflow/github.com/dockstore-testing/md5sum-checker:develop test.json
/home/dyuen/cwltool/venv/bin/cwltool 1.0.20190323055837
Resolved '#workflow/github.com/dockstore-testing/md5sum-checker:develop' to 'https://dockstore.org/api/api/ga4gh/v2/tools/%23workflow%2Fgithub.com%2Fdockstore-testing%2Fmd5sum-checker/versions/develop/plain-CWL/descriptor/md5sum-workflow.cwl'
[workflow ] start
... (snip)
Final process status is success

* resolve imports properly for TRS
@denis-yuen denis-yuen self-assigned this Mar 27, 2019
@mr-c
Copy link
Member

mr-c commented Mar 27, 2019

Thank you @denis-yuen! Can we get a test for the workflow case?

@denis-yuen
Copy link
Member Author

@mr-c I'll give it a shot, but I may need help (not very familiar with Python)
Do you know of a good example test I can work off of? And does this repo normally use mocking for external resources (like a TRS) ?

@denis-yuen denis-yuen changed the title Fix resolver for imports WIP Fix resolver for imports Mar 27, 2019
@codecov
Copy link

codecov bot commented Mar 27, 2019

Codecov Report

Merging #1093 into master will increase coverage by 0.24%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1093      +/-   ##
==========================================
+ Coverage   79.68%   79.92%   +0.24%     
==========================================
  Files          30       30              
  Lines        6137     6152      +15     
  Branches     1534     1537       +3     
==========================================
+ Hits         4890     4917      +27     
+ Misses        870      854      -16     
- Partials      377      381       +4
Impacted Files Coverage Δ
cwltool/resolver.py 77.04% <89.47%> (+33.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e9f82a...4f1572d. Read the comment docs.

@mr-c
Copy link
Member

mr-c commented Mar 28, 2019

Thanks for the integration tests, though judging from Travis CI they either don't work with Py3.4 & 3.5 (unlikely?) or are flaky.

Maybe refactor so that the network query is pulled out from the main logic so it can be mocked out in unit tests?

@denis-yuen
Copy link
Member Author

denis-yuen commented Mar 28, 2019

@mr-c
I'm thinking flaky.

@mr-c
Copy link
Member

mr-c commented Mar 28, 2019

* don't have Windows to test with locally
* looks like it also chokes on WindowsError in a way other versions of
Python don't?
@denis-yuen denis-yuen requested review from tetron and mr-c April 1, 2019 14:21
@denis-yuen
Copy link
Member Author

For reviewers, it looks like remaining errors are false positives since they occur on a different branch too https://ci.commonwl.org/job/cwltool-pr-conformance-multiver/2400/

@tetron
Copy link
Member

tetron commented Apr 1, 2019

jenkins, test this please

@tetron tetron merged commit 0d1fb3f into master Apr 1, 2019
@tetron tetron deleted the trs_workflows branch April 1, 2019 18:45
amarjandu added a commit to dockstore/dockstore-ui2 that referenced this pull request May 10, 2022
amarjandu added a commit to dockstore/dockstore-ui2 that referenced this pull request May 10, 2022
amarjandu added a commit to dockstore/dockstore-ui2 that referenced this pull request May 11, 2022
amarjandu added a commit to dockstore/dockstore-ui2 that referenced this pull request May 11, 2022
denis-yuen pushed a commit to dockstore/dockstore-ui2 that referenced this pull request Jul 15, 2022
denis-yuen added a commit to dockstore/dockstore-ui2 that referenced this pull request Jul 18, 2022
#1529) (#1569)

* Fix: Selectively include CWL instructions (dockstore/dockstore#4862)

* Remove comment for common-workflow-language/cwltool#1093

Co-authored-by: amar jandu <ajandu@ucsc.edu>
denis-yuen pushed a commit to dockstore/dockstore-ui2 that referenced this pull request Aug 12, 2022
denis-yuen added a commit to dockstore/dockstore-ui2 that referenced this pull request Aug 15, 2022
#1592)

* Fix: Selectively include CWL instructions (dockstore/dockstore#4862, PR #1529)

* Fix: Selectively include CWL instructions (dockstore/dockstore#4862)

* Remove comment for common-workflow-language/cwltool#1093

* Update package.json

Co-authored-by: amar jandu <ajandu@ucsc.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants