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

Memory support (Issue #42) #153

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Memory support (Issue #42) #153

wants to merge 9 commits into from

Conversation

ddtm
Copy link
Contributor

@ddtm ddtm commented Jan 24, 2017

Addresses #42.

@mgermain
Copy link
Member

@ddtm Some tests are now failing. Also have you actually tested it?

Copy link
Member

@MarcCote MarcCote left a comment

Choose a reason for hiding this comment

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

A few small typos and comments. Thanks for the PR.

@@ -202,7 +206,9 @@ def parse_arguments():
if args.queueName not in AVAILABLE_QUEUES and ((args.coresPerNode is None and args.gpusPerNode is None) or args.walltime is None):
parser.error("Unknown queue, --coresPerNode/--gpusPerNode and --walltime must be set.")
if args.coresPerCommand < 1:
parser.error("coresPerNode must be at least 1")
parser.error("coresPerCommand must be at least 1")
Copy link
Member

Choose a reason for hiding this comment

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

Thx, good catch.

parser.error("coresPerNode must be at least 1")
parser.error("coresPerCommand must be at least 1")
if args.memPerCommand is not None and args.memPerCommand <= 0:
parser.error("memPerCommand must be positive")
Copy link
Member

Choose a reason for hiding this comment

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

maybe: 'strictly' positive?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah.

Copy link
Member

Choose a reason for hiding this comment

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

oops it is

pbs.add_resources(nodes=resource)

if mem_per_command is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This cannot be None anymore, thanks to the if above.

@@ -60,6 +60,28 @@ def test_generate_pbs4_cpu(self):
# Check if needed modules for this queue are included in the PBS file
assert_equal(job_generator.pbs_list[0].modules, self.modules)

def test_generate_pbs2_mem(self):
# Should need two PBS file
Copy link
Member

Choose a reason for hiding this comment

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

file -> files

assert_equal(job_generator.pbs_list[1].commands, self.commands[2:])

def test_generate_pbs4_mem(self):
# Should needs four PBS file
Copy link
Member

Choose a reason for hiding this comment

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

file -> files

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 94.648% when pulling 110beed on ddtm:memory into c94f08e on SMART-Lab:master.

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.

4 participants