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: remove jobstep #9

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: remove jobstep #9

wants to merge 5 commits into from

Conversation

BEFH
Copy link
Owner

@BEFH BEFH commented Jun 11, 2024

Features:

  • Better support for MPI and span strings

Fixes:

  • Remove unnecessary jobstep executor
  • Change generator type hint
  • Move some code out of the main Executor method

@BEFH BEFH changed the title Remove jobstep fix: remove jobstep Jun 11, 2024
@BEFH
Copy link
Owner Author

BEFH commented Jun 11, 2024

@cmeesters I have integrated (and tested) some of our suggested changes. Could you please particularly take a look at what I've done for the span arguments?

@cmeesters
Copy link

Hi Brian,

pew, sorry, I was busy.

I think there was a misunderstanding: You still need the jobstep executor. Just like in lines 58 here.

The Jobstep Executor in turn calls Snakemake again. Except for the MPI case, where merely the shell command is executed. What you do not need in the LSF case is a handling of a true jobstep like in SLURM.

Does this make sense to you?

@BEFH
Copy link
Owner Author

BEFH commented Jun 17, 2024

I tested this and it seems to be working without the jobstep. I'll post the test results shortly.

@cmeesters
Copy link

I doubt this: for ordinary SMP jobs and group jobs, yes. But for MPI Jobs?

@BEFH
Copy link
Owner Author

BEFH commented Jun 17, 2024

On LSF, MPI commands are just run from within jobs like any other commands, so I think it should be fine for MPI.

But if you point me to your pi test workflow, I can get confirmation one way or another.

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.

2 participants