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

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

Merged
merged 7 commits into from Oct 25, 2021

Conversation

DonFreed
Copy link
Contributor

@DonFreed DonFreed commented Oct 12, 2021

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 Sep 22, 2021
- 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
@DonFreed DonFreed requested a review from johanneskoester as a code owner Oct 12, 2021
Copy link
Contributor

@johanneskoester johanneskoester left a comment

Thanks a lot!

@johanneskoester
Copy link
Contributor

johanneskoester commented Oct 21, 2021

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

@sonarcloud
Copy link

sonarcloud bot commented Oct 23, 2021

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
Contributor Author

DonFreed commented Oct 23, 2021

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
6 checks passed
@DonFreed DonFreed deleted the fix_checkpoint_finishing_823 branch Oct 26, 2021
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.

None yet

3 participants