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: fix string resource definition in CLI and profile #2627

Merged
merged 7 commits into from Feb 2, 2024

Conversation

cmeesters
Copy link
Contributor

@cmeesters cmeesters commented Jan 17, 2024

Description

The PR is related to the issue described for the slurm submit executor plugin to not correctly hand over arguments.

The test folder is named test_slurm_resource_propagation meesters and may not be complete, as it covers multiple related scenarios to provoke the error.

Note, that the tests were carried out when using a cluster:

  • $ snakemake --executor slurm -j2 --workflow-profile ./profiles/ --default-resources slurm_account=<account>
    yields
    ``Error:
    WorkflowError:
    Failed to evaluate default resources value '<function eval_resource_expression..callable at 0x7f5c19ed0e00>'.```

    when the patition is defined at the rule level in a profile. (see the directory of the test case)

  • likewise $ snakemake --executor slurm -j2 --workflow-profile ./profiles/ --default-resources slurm_account=<account> --set-resources slurm_extra="--nice=150"
    yields
    ValueError: Invalid resource definition: entries have to be defined as RULE:RESOURCE=VALUE, with VALUE being a positive integer a quoted string, or a Python expression (e.g. min(max(2*input.size_mb, 1000), 8000)).

These errors can both be triggered on the command line and in the config file, of course.

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

@cmeesters
Copy link
Contributor Author

@johanneskoester does this make sense? Any progress on this side?

@johanneskoester johanneskoester changed the title Fix/default arg propagation fix: fix string resource definition in CLI and profile Feb 2, 2024
Copy link

sonarcloud bot commented Feb 2, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@johanneskoester johanneskoester merged commit bbd76ae into main Feb 2, 2024
34 checks passed
@johanneskoester johanneskoester deleted the fix/default-arg-propagation branch February 2, 2024 10:37
johanneskoester pushed a commit that referenced this pull request Feb 2, 2024
🤖 I have created a release *beep* *boop*
---


##
[8.4.3](v8.4.2...v8.4.3)
(2024-02-02)


### Bug Fixes

* Do not scheduler execution message if no jobs are ready
([b1c4f47](b1c4f47))
* fix string resource definition in CLI and profile
([#2627](#2627))
([bbd76ae](bbd76ae))
* if report files are within storage, retrieve them from storage before
loading into report
([60041bd](60041bd))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@cmeesters
Copy link
Contributor Author

Thank you for the fix! Alas, case no. 1 is not covered, yet - as it seems.

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