Skip to content

fix: Adds fixes for the first two MREs in #823#1215

Merged
johanneskoester merged 7 commits intosnakemake:mainfrom
DonFreed:fix_checkpoint_finishing_823
Oct 25, 2021
Merged

fix: Adds fixes for the first two MREs in #823#1215
johanneskoester merged 7 commits intosnakemake:mainfrom
DonFreed:fix_checkpoint_finishing_823

Conversation

@DonFreed
Copy link
Copy Markdown
Contributor

Description

This PR extends @epruesse's fix in #1203. Importantly, it adds some changes to ensure that group jobs are marked finished, which fixes the second MRE in #823.

Together, the changes fix the first and second MREs in #823.

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

Maarten-vd-Sande and others added 4 commits September 22, 2021 12:12
- Incorporates @epruesse's fix for MRE snakemake#1
- Adds a fix for MRE snakemake#2 - properly marks group jobs as finished
- Some minor updates to tests
Copy link
Copy Markdown
Contributor

@johanneskoester johanneskoester left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@johanneskoester
Copy link
Copy Markdown
Contributor

The tests seem to fail on windows for some reason. Can you have a look?

@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@DonFreed
Copy link
Copy Markdown
Contributor Author

The second MRE was failing with due to the pipe() output.

I think the piped output is required for reproducing the behavior in #823, so it seems appropriate to skip the Windows test for now. It should be OK to remove the @skip_on_windows when pipe() is supported on Windows.

@johanneskoester johanneskoester merged commit cfd2f89 into snakemake:main Oct 25, 2021
@DonFreed DonFreed deleted the fix_checkpoint_finishing_823 branch October 26, 2021 04:27
pvandyken pushed a commit to pvandyken/snakemake that referenced this pull request Nov 15, 2021
* add failing tests 823

* fix mistakes

* black

* Fix the first two MREs from snakemake#823.

- Incorporates @epruesse's fix for MRE #1
- Adds a fix for MRE #2 - properly marks group jobs as finished
- Some minor updates to tests

* Fix tests on Windows

* Skip MRE 2 from 823 on Windows due to `pipe()` output

Co-authored-by: Maarten-vd-Sande <maartenvandersande@hotmail.com>
Co-authored-by: Johannes Köster <johannes.koester@uni-due.de>
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