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

feat: add wildcards to output and comment #35

Merged
merged 5 commits into from
Feb 29, 2024
Merged

Conversation

Ulthran
Copy link
Contributor

@Ulthran Ulthran commented Feb 13, 2024

Add rule wildcards to slurm output logs and job comments. Not sure if this will need extra checks for when job is a group instead of a rule.

cmeesters
cmeesters previously approved these changes Feb 14, 2024
Copy link
Collaborator

@cmeesters cmeesters left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution. Did not find the time for it, yet. You simply did it!

@cmeesters cmeesters dismissed their stale review February 14, 2024 13:08

group jobs do not have a wildcard attribute

Copy link
Collaborator

@cmeesters cmeesters left a comment

Choose a reason for hiding this comment

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

AttributeError: 'GroupJob' object has no attribute 'wildcards'

@cmeesters
Copy link
Collaborator

I'm afraid, that's not possible this way. Also: is adding arbitrary wildcards really desirable? Or just the rule name?

@Ulthran
Copy link
Contributor Author

Ulthran commented Feb 14, 2024

Can we only include the wildcards if it is a Job type and pass an empty string if it's a GroupJob? Tried implementing that in the most recent commit.

In our case, we would always want to have all wildcards included in the output names. Is your concern that there might be cases where there are too many wildcards for this to be reasonable?

Copy link
Collaborator

@cmeesters cmeesters left a comment

Choose a reason for hiding this comment

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

passes, works as desired

@cmeesters
Copy link
Collaborator

@Ulthran no, just fix the linting issue(s), please. I recommend running black on snakemake code, before commits.

I have no concerns in that regard: If people overdo appending wildcards, eventually SLURM will raise errors, but that is a very minor concern ;-)

@cmeesters cmeesters merged commit 190500b into snakemake:main Feb 29, 2024
4 checks passed
cmeesters pushed a commit that referenced this pull request Feb 29, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.4.0](v0.3.2...v0.4.0)
(2024-02-29)


### Features

* add wildcards to output and comment
([#35](#35))
([190500b](190500b))

---
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>
@jaicher
Copy link

jaicher commented Mar 4, 2024

This breaks pipelines where wildcards can include the directory separator "/".

It should be straightforward to fix by sanitizing the wildcard string used in the logfile name to remove/replace the extra directory separators.

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