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

Snakemake upgrade #471

Merged
merged 2 commits into from
Oct 28, 2024
Merged

Snakemake upgrade #471

merged 2 commits into from
Oct 28, 2024

Conversation

Alputer
Copy link
Member

@Alputer Alputer commented Oct 8, 2024

No description provided.

@Alputer Alputer self-assigned this Oct 8, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 15, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 15, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 15, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024
) as snakemake_api:
try:
workflow_api = snakemake_api.workflow(
resource_settings=ResourceSettings(nodes=300),
Copy link
Member

Choose a reason for hiding this comment

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

For the nodes settings, we already have SNAKEMAKE_MAX_PARALLEL_JOBS environmental variable that could be used instead of the hard-coded value of 300, even though this might "fully' work only when the validation would be moved to the server-side.

Still, you could preventively introduce in reana_commons/config.py something like:

SNAKEMAKE_MAX_PARALLEL_JOBS = int(os.getenv("SNAKEMAKE_MAX_PARALLEL_JOBS", "300"))
"""Snakemake maximum number of jobs that can run in parallel."""

which might be better that hard-coding the value of 300.

(Similarly as we do in reana_workflow_engine_snakemake/config.py.)

setup.py Outdated
# see https://github.com/snakemake/snakemake/issues/2657
"snakemake @ git+https://github.com/mdonadoni/snakemake.git@cea31624976989ad0645eb2e1751260d32259506", # branch `7.32.4-python3.12`
"pulp>=2.7.0,<2.8.0",
"snakemake==8.23.0",
Copy link
Member

Choose a reason for hiding this comment

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

There is already Snakemake 8.24.0 out.

workdir=workdir,
)
# Try to create dag and it throws an error in case of a failure.
# Seems to be enough but might not be an extensive validation.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps amend the docstring to:

Seems to be enough for the first validation. We may move to snakemake --dry-run when the validation process will be fully moved to the server-side.

Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024
Copy link

codecov bot commented Oct 23, 2024

Codecov Report

Attention: Patch coverage is 63.41463% with 15 lines in your changes missing coverage. Please review.

Project coverage is 41.79%. Comparing base (564c027) to head (3b80320).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
reana_commons/snakemake.py 68.42% 12 Missing ⚠️
reana_commons/config.py 0.00% 2 Missing ⚠️
reana_commons/version.py 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #471      +/-   ##
==========================================
- Coverage   44.23%   41.79%   -2.45%     
==========================================
  Files          28       28              
  Lines        1978     2010      +32     
==========================================
- Hits          875      840      -35     
- Misses       1103     1170      +67     
Files with missing lines Coverage Δ
reana_commons/version.py 0.00% <0.00%> (ø)
reana_commons/config.py 0.00% <0.00%> (ø)
reana_commons/snakemake.py 30.09% <68.42%> (-60.32%) ⬇️

Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 23, 2024

from reana_commons.snakemake import snakemake_load


@pytest.mark.xfail(
sys.version_info >= (3, 11),
reason="Test expted to fail for python versions 3.11 and above as we currently return only empty dictionary in snakemake_load function for python >= 3.11. Development is blocked until we move validation to the server side.",
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "expected" to fail

Copy link
Member

Choose a reason for hiding this comment

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

You can also remove the last part "Development is blocked until we move validation to the server side" to make the reason shorter. We'll revamp this anyway as part of the server-side validation sprint.

@@ -124,7 +124,7 @@ jobs:
- name: Install system dependencies
run: |
sudo apt-get update -y
sudo apt-get install python3-dev graphviz libgraphviz-dev pkg-config
sudo apt-get install python3-dev graphviz libgraphviz-dev pkg-config uuid-dev
Copy link
Member

Choose a reason for hiding this comment

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

This is necessary to make the CI pass for the Snakemake v8 upgrade, so I would just squash the two functional commits together under the heading refactor(snakemake): upgrade snakemake 7 to snakemake 8 (#471) and add only the separate release commit on top.



def snakemake_validate(
workflow_file: str, configfiles: List[str], workdir: Optional[str] = None
):
"""Validate Snakemake workflow specification.
"""Validate snakemake workflow."""
Copy link
Member

Choose a reason for hiding this comment

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

Cosmetics: Validate Snakemake workflow. (note upppercase)

def snakemake_validate_v7(
workflow_file: str, configfiles: List[str], workdir: Optional[str] = None
):
"""Old snakemake validate version for python versions < 3.11, it is needed since snakemake8 dropped support for python < 3.11.
Copy link
Member

Choose a reason for hiding this comment

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

Snakemake 7 workflow validation function, necessary for Python versions < 3.11.

(to make the docstring shorter)

def snakemake_validate_v8(
workflow_file: str, configfiles: List[str], workdir: Optional[str] = None
):
"""Seems to be enough for the first validation. We may move to snakemake --dry-run when the validation process will be fully moved to the server-side.
Copy link
Member

Choose a reason for hiding this comment

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

Snakemake 8 workflow validation function for Python versions >= 3.11.

Note that we may move to using snakemake --dry-run when the validation process will be fully moved to the server side.

(to make the headline shorter, whilst the comment goes to the docstring body.)



def snakemake_load(workflow_file: str, **kwargs: Any):
"""Load snakemake specification."""
Copy link
Member

Choose a reason for hiding this comment

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

Cosmetics: Load Snakemake specification. (note uppercase)

Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 28, 2024
Alputer added a commit to Alputer/reana-commons that referenced this pull request Oct 28, 2024
Copy link
Member

@tiborsimko tiborsimko left a comment

Choose a reason for hiding this comment

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

Works nicely for the full Snakemake test matrix 👍

@tiborsimko tiborsimko merged commit 3b80320 into reanahub:master Oct 28, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants