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

Add default resource params and slurm destination #61

Merged
merged 3 commits into from
May 28, 2024

Conversation

nuwang
Copy link
Member

@nuwang nuwang commented Apr 29, 2024

@nuwang nuwang requested a review from cat-bro April 29, 2024 03:30
tools.yml Outdated
@@ -10,6 +12,8 @@ tools:
rules: []
rank: |
helpers.weighted_random_sampling(candidate_destinations)
default:
extends: tpvdb_default
Copy link
Collaborator

Choose a reason for hiding this comment

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

if default inherits tbvdb_default, what do this do that is different?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was to preserve backward compatibility, but I think that this may end up just creating an infinite loop. We probably want this these changes to spread to all destinations anyway, so I'll change this back.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@cat-bro
Copy link
Collaborator

cat-bro commented Apr 29, 2024

This looks good. The way it is structured, would it be an opt-out rather than opt-in setup?

@nuwang nuwang force-pushed the imporve_default_entities branch 2 times, most recently from 5cd448c to c204c7e Compare April 30, 2024 06:02
tools.yml Outdated
abstract: true
tpvdb_slurm:
context:
time: ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if these variable names are too generic. 'time' in particular could collide with python's time library

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. I've renamed time for now, but perhaps we could just prefix everything with slurm_ instead?

Copy link
Member

Choose a reason for hiding this comment

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

I like the prefixing idea.

tools.yml Outdated
Comment on lines 2572 to 2580
context:
time: ''
partition: ''
account: ''
params:
native_specification: --nodes=1 --ntasks={cores} --mem={round(mem*1024)} {'--time='
+ time if time else ''} {'--gres=gres:gpu:' + str(gpus) if gpus else ''} {'--partition='
+ partition if partition else ''} {'--account=' + account if account else
''}
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
context:
time: ''
partition: ''
account: ''
params:
native_specification: --nodes=1 --ntasks={cores} --mem={round(mem*1024)} {'--time='
+ time if time else ''} {'--gres=gres:gpu:' + str(gpus) if gpus else ''} {'--partition='
+ partition if partition else ''} {'--account=' + account if account else
''}
context:
walltime: ''
partition: ''
account: ''
params:
native_specification: --nodes=1 --ntasks={cores} --mem={round(mem*1024)} {'--time='
+ walltime if walltime else ''} {'--gres=gres:gpu:' + str(gpus) if gpus else ''} {'--partition='
+ partition if partition else ''} {'--account=' + account if account else
''}

tools.yml Outdated
abstract: true
tpvdb_slurm:
context:
time: ''
Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. I've renamed time for now, but perhaps we could just prefix everything with slurm_ instead?

tools.yml Outdated
partition: ''
account: ''
params:
native_specification: --nodes=1 --ntasks={cores} --mem={round(mem*1024)} {'--time='
Copy link
Collaborator

Choose a reason for hiding this comment

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

given that this is multiline, it could be clearer to use one argument per line
--nodes=1
--ntasks={cores}
etc

Copy link
Member Author

@nuwang nuwang May 1, 2024

Choose a reason for hiding this comment

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

This was because oftpv format. I've tried an alternative formatting that at least preserves everything in one, very lengthy line. Setting the block chomping indicator to strip newlines also gets reformatted, so the only options appear to be either a single lengthy line or multiple lines broken up as above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. I hadn't thought about tpv format. This looks good as is

@nuwang nuwang requested review from natefoo and bgruening May 1, 2024 06:31
@bgruening
Copy link
Member

I'm not familiar with SLURM, to me it looks good. Thanks @nuwang

@nuwang nuwang merged commit e3f3629 into main May 28, 2024
1 check passed
@nuwang nuwang deleted the imporve_default_entities branch May 28, 2024 07:48
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.

Add info about cores, mem and gpus to a job in a standard way
3 participants