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.

Sign up for GitHub

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

Jump to bottom

feat: multicluster #56

Merged
merged 27 commits into from
Aug 6, 2024
Merged

feat: multicluster #56

merged 27 commits into from
Aug 6, 2024

Conversation

cmeesters
Copy link
Member

Putative fix for issue #53

@cmeesters cmeesters changed the title Feat/multicluster feat: multicluster Apr 2, 2024
@cmeesters cmeesters mentioned this pull request Apr 2, 2024
Closed
@meliamne
Copy link

meliamne commented Apr 7, 2024

when specifying the cluster in sbatch the output changes to Submitted batch job 61317320 on cluster wice. This causes line 162 to mistakenly pick up the name of the cluster instead of the jobid.

snakemake-executor-plugin-slurm/snakemake_executor_plugin_slurm/__init__.py

Line 162 in 7103c7e

slurm_jobid = out.split(" ")[-1]

@cmeesters
Copy link
Member Author

@meliamne Thank you, your suggestion went into my last commit.

@cmeesters
Copy link
Member Author

@johanneskoester does the current state find your approval?

I dropped

  • self.__clusters as an attribute, instead
  • sacct and scancel use --clusters=all, as suggested.
  • in addition, please take a look at the regex use in line 159 .

@cmeesters
Copy link
Member Author

@johanneskoester we do not have a test, yet (due to faulty multicluster setup in the test-engine). Should we merge regardless, considering that the test will take considerable time and effort to implement?

@cmeesters cmeesters requested a review from johanneskoester May 7, 2024 19:01
@cmeesters
Copy link
Member Author

@johanneskoester not sure, isn't the requested change already in there, resp. the issue solved?

cmeesters and others added 7 commits July 5, 2024 15:10
@cmeesters
fix: merge conflict
bef2059
@github-actions @cmeesters
chore(main): release 0.6.0 (#100)
03a22e6 
🤖 I have created a release *beep* *boop*
---


##
[0.6.0](v0.5.2...v0.6.0)
(2024-06-07)


### Features

* will reject jobs, which attempt setting job names by 'slurm_extra'
([#93](#93))
([df2fd3d](df2fd3d))

---
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
docs: added paragraphs about dynamic resource allocation (#79)
c6ed8a8
@cmeesters
docs: storage update (#80)
ffff12f 
reflects update in storage plugin fs
@cmeesters
feat: warning if run in job (#78)
b5d66fd 
Already, two issues (#75 and #22) seem to result from running Snakemake
in a SLURM job context and using this executor plugin. This PR
introduces detecting if triggered within a SLURM job and issuing a
warning accordingly.

In principle, the plugin may work in job context. Submitting jobs from
jobs has always been a highlight of SLURM. However, settings may lead to
unintended behaviour (and would do so without Snakemake, presumably).
Hence, we can only warn from the executor.
@cmeesters
fix: null byte account guess (#81)
de7251e 
attempt to ensure that a '(null)' byte does not overthrow the account
testing.
@cmeesters @johanneskoester
docs: added mini paragraph about Conda and Env Modules (#42)
385a592 
Just a mini change, writing hints about software usage.

---------

Co-authored-by: Johannes Köster <johannes.koester@uni-due.de>
github-actions bot and others added 6 commits July 5, 2024 15:12
@github-actions @cmeesters
chore(main): release 0.7.0 (#108)
a97fc94 
🤖 I have created a release *beep* *boop*
---


##
[0.7.0](v0.6.0...v0.7.0)
(2024-06-25)


### Features

* warning if run in job
([#78](#78))
([257e830](257e830))


### Bug Fixes

* null byte account guess
([#81](#81))
([92d4445](92d4445))


### Documentation

* added mini paragraph about Conda and Env Modules
([#42](#42))
([c821b5e](c821b5e))
* added paragraphs about dynamic resource allocation
([#79](#79))
([06a1555](06a1555))
* storage update
([#80](#80))
([7e19560](7e19560))

---
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
fix: added comment to jobid extraction and made a little more reliant.
0958e88
@cmeesters
docs: added paragraphs about dynamic resource allocation (#79)
a237498
@cmeesters
fix: syntax
9287e62
@cmeesters
gnarf: again fixing merge conflict
8567f16
@cmeesters
fix: merge conflict
4be82b5
@cmeesters
Copy link
Member Author

@johanneskoester please have a look, after all this rebasing and fixing, I am a little puzzled. It's embarrassing, but it took too long.

@cmeesters
Copy link
Member Author

We (@johanneskoester ) and me discussed that we do not have a solution for a multicluster test at hand for the time being. Hence, this PR is merged without a dedicated test.

I am sorry, I could merge this sooner. Too many things kept coming up, then holidays ....

@cmeesters cmeesters merged commit c0f8fee into main Aug 6, 2024
4 checks passed
@cmeesters cmeesters deleted the feat/multicluster branch August 6, 2024 07:49
@github-actions github-actions bot mentioned this pull request Aug 6, 2024
Merged
cmeesters pushed a commit that referenced this pull request Aug 6, 2024
@github-actions
chore(main): release 0.9.0 (#124)
03d474f 
🤖 I have created a release *beep* *boop*
---


##
[0.9.0](v0.8.0...v0.9.0)
(2024-08-06)


### Features

* multicluster
([#56](#56))
([c0f8fee](c0f8fee))


### Bug Fixes

* fixed string for constraints - see issue
[#58](#58)
([#64](#64))
([89e10ff](89e10ff))

---
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>
This was referenced Aug 6, 2024
Closed
Closed
@nigiord
Copy link
Contributor

nigiord commented Aug 6, 2024

Thanks for this merge! I think there is a mistake in the documentation though:

snakemake-executor-plugin-slurm/docs/further.md

Line 122 in 03d474f

| `--clusters` | `cluster` | comma separated string of clusters |

The snakemake resources seems to be clusters (with an s), and not cluster:

snakemake-executor-plugin-slurm/snakemake_executor_plugin_slurm/__init__.py

Line 140 in 03d474f

if job.resources.get("clusters"):

I also made a comment regarding the way jobid is now parsed: #121 (comment)

Cheers,

@cmeesters
Copy link
Member Author

Ah, thanks. Will fix this asap (= not today).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meliamne meliamne meliamne left review comments

@johanneskoester johanneskoester Awaiting requested review from johanneskoester

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cmeesters @meliamne @nigiord @johanneskoester