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: enable usage of job grouping in GLS #1054

Merged
merged 6 commits into from Oct 21, 2021

Conversation

sposadac
Copy link
Contributor

@sposadac sposadac commented Jun 17, 2021

Description

Allow using Google Life Science preemptible rules and job grouping, by checking if any of the rules in a given group has been flagged for execution using preemptible instances.

QC

  • 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).

@sposadac sposadac requested a review from johanneskoester as a code owner Jun 17, 2021
@sonarcloud
Copy link

sonarcloud bot commented Jun 17, 2021

Kudos, SonarCloud 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

@@ -1331,6 +1331,8 @@ However, if we would add ``group: "mygroup"`` to rule ``c``, all jobs would end
Alternatively, groups can be defined via the command line interface.
This enables to almost arbitrarily partition the DAG, e.g. in order to safe network traffic, see :ref:`here <job_grouping>`.

For execution on the cloud using Google Life Science API and preemptible instances, note than if any of the rules in a group is included in the subset of rules to be executed using preembtible instances (with command line option ``--preemptible-rules``), the group will be also scheduled to be executed on a preemptible instance.
Copy link
Contributor

@johanneskoester johanneskoester Jun 21, 2021

Choose a reason for hiding this comment

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

Mhm, I wonder if that is really the desired behavior? Shouldn't it rather be: all jobs must be preemptible for the group to be preemptible as well?

Copy link
Contributor Author

@sposadac sposadac Jun 21, 2021

Choose a reason for hiding this comment

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

I don't have a strong opinion, my only thought went into cost optimization. If one were to mark a rule as "preemptible" because being somehow expensive, I would like to make sure is executed in a preemptible instance.

That said, I wouldn't mind to replace the any by the all condition. Let me know if it is still your preference

Copy link
Contributor

@johanneskoester johanneskoester Jul 5, 2021

Choose a reason for hiding this comment

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

THinking about it, we could also fail with a WorkflowError in case not all jobs are preemptible, and request people to set this homogeneously for all jobs in the group, pointing to the corresponding section in the docs.

Copy link
Contributor

@johanneskoester johanneskoester Jul 5, 2021

Choose a reason for hiding this comment

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

I think this is best, because peoples credits depend on that behavior, so we'd rather not imply something that will cost more money than expected.

Copy link
Contributor Author

@sposadac sposadac Aug 4, 2021

Choose a reason for hiding this comment

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

It sounds reasonable to me! I implemented the few edits.

@sposadac sposadac requested a review from johanneskoester Aug 4, 2021
@sposadac sposadac changed the title Groupjobs gls Enable usage of job grouping in GLS Aug 5, 2021
@sposadac sposadac changed the title Enable usage of job grouping in GLS fix: enable usage of job grouping in GLS Aug 6, 2021
@sonarcloud
Copy link

sonarcloud bot commented Aug 12, 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

@sposadac
Copy link
Contributor Author

sposadac commented Aug 20, 2021

Hi @johanneskoester,

Thanks for approving the changes! Since I am not authorized to merge the PR, I am wondering what would be the next step.

@sposadac sposadac requested a review from johanneskoester Oct 19, 2021
@johanneskoester
Copy link
Contributor

johanneskoester commented Oct 21, 2021

Merging! Sorry, I was a bit busy in the last weeks.

@johanneskoester johanneskoester merged commit d243c22 into snakemake:main Oct 21, 2021
6 checks passed
pvandyken pushed a commit to pvandyken/snakemake that referenced this pull request Nov 15, 2021
* Allow using grouped rules on Google cloud using Google Life Science
API

* Explain new behaviour in the docs

* Modify behaviour to request all rules in a group to be set as preemptible instances

* Fix typo and add comment about documentation

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

2 participants