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

Add slurm runner #799

Merged
merged 82 commits into from
Apr 2, 2020
Merged

Add slurm runner #799

merged 82 commits into from
Apr 2, 2020

Conversation

JayjeetAtGithub
Copy link
Collaborator

@JayjeetAtGithub JayjeetAtGithub commented Mar 20, 2020 •
edited
Loading

Adds basic Slurm resource manager support

@pep8speaks
Copy link

pep8speaks commented Mar 20, 2020 •
edited
Loading

Hello @JayjeetAtGithub! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 55:80: E501 line too long (84 > 79 characters)
Line 56:80: E501 line too long (84 > 79 characters)

Line 181:80: E501 line too long (88 > 79 characters)

Line 71:80: E501 line too long (102 > 79 characters)

Line 109:80: E501 line too long (159 > 79 characters)
Line 115:80: E501 line too long (158 > 79 characters)
Line 122:80: E501 line too long (158 > 79 characters)
Line 130:80: E501 line too long (158 > 79 characters)

Comment last updated at 2020-04-01 15:30:26 UTC

@JayjeetAtGithub JayjeetAtGithub force-pushed the jayjeet/runner_slurm branch 3 times, most recently from 45fd329 to ed5ffe4 Compare March 20, 2020 19:07
@JayjeetAtGithub JayjeetAtGithub changed the title Jayjeet/runner slurm Add slurm runner Mar 20, 2020
@JayjeetAtGithub JayjeetAtGithub requested a review from ivotron March 20, 2020 19:52
@JayjeetAtGithub JayjeetAtGithub marked this pull request as ready for review March 20, 2020 19:52
ivotron
ivotron requested changes Mar 21, 2020
View reviewed changes
Copy link
Collaborator

@ivotron ivotron left a comment

Choose a reason for hiding this comment

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

thanks a lot @JayjeetAtGithub, this is looking great! I think the only thing that would be missing would be to add tests.

cli/popper/runner_slurm.py Outdated Show resolved Hide resolved
cli/popper/runner_slurm.py Outdated Show resolved Hide resolved
@JayjeetAtGithub
Copy link
Collaborator Author

thanks a lot @JayjeetAtGithub, this is looking great! I think the only thing that would be missing would be to add tests.

Yeah. Would be pushing tests soon.

@codecov
Copy link

codecov bot commented Mar 21, 2020 •
edited
Loading

Codecov Report

Merging #799 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #799   +/-   ##
=======================================
  Coverage   86.58%   86.58%           
=======================================
  Files          18       18           
  Lines        1699     1699           
=======================================
  Hits         1471     1471           
  Misses        228      228

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18829ed...18829ed. Read the comment docs.

@JayjeetAtGithub JayjeetAtGithub requested a review from ivotron March 23, 2020 04:58
ivotron
ivotron requested changes Mar 23, 2020
View reviewed changes
Copy link
Collaborator

@ivotron ivotron left a comment

Choose a reason for hiding this comment

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

thanks a lot! please take a look at the comments

cli/popper/runner.py Outdated Show resolved Hide resolved
ci/run_tests.sh Outdated Show resolved Hide resolved
ci/test/engine-conf Show resolved Hide resolved
ci/test/slurm Outdated Show resolved Hide resolved
cli/popper/runner.py Outdated Show resolved Hide resolved
cli/popper/runner_slurm.py Outdated Show resolved Hide resolved
cli/popper/runner_slurm.py Show resolved Hide resolved
@JayjeetAtGithub JayjeetAtGithub force-pushed the jayjeet/runner_slurm branch 3 times, most recently from 19d5a00 to 6d7ecbf Compare March 31, 2020 12:15
ivotron
ivotron requested changes Mar 31, 2020
View reviewed changes
Copy link
Collaborator

@ivotron ivotron left a comment

Choose a reason for hiding this comment

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

excellent! I think we only need tests and this will be done. thanks a lot for all your patience @JayjeetAtGithub !

cli/popper/utils.py Outdated Show resolved Hide resolved
cli/popper/utils.py Outdated Show resolved Hide resolved
cli/popper/runner_slurm.py Outdated Show resolved Hide resolved
cli/popper/runner_slurm.py Outdated Show resolved Hide resolved
cli/popper/runner.py Show resolved Hide resolved
cli/test/test_parser.py Outdated Show resolved Hide resolved
@JayjeetAtGithub JayjeetAtGithub requested a review from ivotron April 1, 2020 15:40
ivotron
ivotron approved these changes Apr 2, 2020
View reviewed changes
Copy link
Collaborator

@ivotron ivotron left a comment

Choose a reason for hiding this comment

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

thanks a lot!

@ivotron ivotron merged commit b59bf85 into master Apr 2, 2020
@ivotron ivotron deleted the jayjeet/runner_slurm branch April 2, 2020 06:07
ivotron pushed a commit that referenced this pull request May 24, 2020
@JayjeetAtGithub
adds slurm runner (#799)
81ddb8f 
- adds slurm runner, with an initial implementation for docker
- refactors runner_host.DockerRunner so children classes can reuse most of the code
- introduces a PopperConfig class that deals with configuration of Popper
- adds tests for slurm runner on docker
ivotron pushed a commit that referenced this pull request May 25, 2020
@JayjeetAtGithub
adds slurm runner (#799)
b24c8cd 
- adds slurm runner, with an initial implementation for docker
- refactors runner_host.DockerRunner so children classes can reuse most of the code
- introduces a PopperConfig class that deals with configuration of Popper
- adds tests for slurm runner on docker
ivotron pushed a commit that referenced this pull request May 25, 2020
@JayjeetAtGithub
adds slurm runner (#799)
7e77db0 
- adds slurm runner, with an initial implementation for docker
- refactors runner_host.DockerRunner so children classes can reuse most of the code
- introduces a PopperConfig class that deals with configuration of Popper
- adds tests for slurm runner on docker
ivotron pushed a commit that referenced this pull request May 25, 2020
@JayjeetAtGithub
adds slurm runner (#799)
61c056c 
- adds slurm runner, with an initial implementation for docker
- refactors runner_host.DockerRunner so children classes can reuse most of the code
- introduces a PopperConfig class that deals with configuration of Popper
- adds tests for slurm runner on docker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivotron ivotron ivotron approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@JayjeetAtGithub @pep8speaks @ivotron