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

Remove --disableCaching's argument #3869

Merged
merged 12 commits into from
Nov 21, 2021

Conversation

adamnovak
Copy link
Member

@adamnovak adamnovak commented Oct 22, 2021

This will fix #3831 by making the --disableCaching usage shown there the only acceptable one. As is, --disableCaching will try to eat the first positional argument if it comes last before the positional arguments, and will break command lines that look like they should work.

Changelog Entry

To be copied to the draft changelog by merger:

  • Breaking change! --disableCaching True/False and --disableCaching=True/False command line argument forms are no longer accepted. Just pass or do not pass --disableCaching like the other --disableX flags.

Reviewer Checklist

  • Make sure it is coming from issues/XXXX-fix-the-thing in the Toil repo, or from an external repo.
    • If it is coming from an external repo, make sure to pull it in for CI with:
      contrib/admin/test-pr otheruser theirbranchname issues/XXXX-fix-the-thing
      
    • If there is no associated issue, create one.
  • Read through the code changes. Make sure that it doesn't have:
    • Addition of trailing whitespace.
    • New variable or member names in camelCase that want to be in snake_case.
    • New functions without type hints.
    • New functions or classes without informative docstrings.
    • Changes to semantics not reflected in the relevant docstrings.
    • New or changed command line options for Toil workflows that are not reflected in docs/running/{cliOptions,cwl,wdl}.rst
    • New features without tests.
  • Comment on the lines of code where problems exist with a review comment. You can shift-click the line numbers in the diff to select multiple lines.
  • Finish the review with an overall description of your opinion.

Merger Checklist

  • Make sure the PR passes tests.
  • Make sure the PR has been reviewed since its last modification. If not, review it.
  • Merge with the Github "Squash and merge" feature.
    • If there are multiple authors' commits, add Co-authored-by to give credit to all contributing authors.
  • Copy its recommended changelog entry to the Draft Changelog.
  • Append the issue number in parentheses to the changelog entry.

@DailyDreaming
Copy link
Member

________________ ERROR collecting src/toil/test/cwl/cwlTest.py _________________
Traceback (most recent call last):
  File "/usr/lib/python3.6/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 994, in _gcd_import
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 665, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 674, in exec_module
  File "<frozen importlib._bootstrap_external>", line 781, in get_code
  File "<frozen importlib._bootstrap_external>", line 741, in source_to_code
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/builds/databiosphere/toil/src/toil/test/cwl/cwlTest.py", line 95
    args_passed_directly_to_toil = (['--disableCaching'] if not caching else []) +
                                                                                 ^
SyntaxError: invalid syntax

@adamnovak
Copy link
Member Author

OK I think I've now learned to syntax.

@adamnovak
Copy link
Member Author

New Gitlab didn't pick this up automatically so I started a pipeline at https://ucsc-ci.com/databiosphere/toil/-/pipelines/108.

@adamnovak
Copy link
Member Author

Looks like https://ucsc-ci.com/databiosphere/toil/-/jobs/1959 had a Kubernetes CWL job randomly time out again, and managed to not emit any JUnit files with the logs. We should be using a cwlrunner that reports logs for timed-out tests, but we don't seem to have any JUnit output this time.

@mr-c Any idea how that can happen?

@mr-c
Copy link
Contributor

mr-c commented Nov 18, 2021

@mr-c Any idea how that can happen?

I don't know. I made a new cwltest release that includes your fixes for timed-out tests. https://github.com/common-workflow-language/cwltest/releases/tag/2.2.20211116163652 a.k.a. https://pypi.org/project/cwltest/2.2.20211116163652/

Copy link
Member

@DailyDreaming DailyDreaming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I've restarted the tests.

@DailyDreaming DailyDreaming merged commit c4dc613 into master Nov 21, 2021
@DailyDreaming DailyDreaming deleted the issues/3831-disableCaching-argument branch November 21, 2021 07:25
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.

Option parse error with --disableCaching
3 participants