Skip to content

Conversation

@teojgo
Copy link
Contributor

@teojgo teojgo commented Jun 15, 2020

Fixes #1032

@teojgo teojgo added this to the ReFrame sprint 20.09 milestone Jun 15, 2020
@teojgo teojgo requested review from ekouts and vkarak June 15, 2020 14:59
@teojgo teojgo self-assigned this Jun 15, 2020
* Add unittests

* Update documentation
@teojgo teojgo force-pushed the feat/flex_alloc_nodes_maint branch from bb3446a to bfbc02d Compare June 15, 2020 15:06
@vkarak vkarak changed the title [feat] Feat/flex alloc nodes maint [feat] Add a maintenance mode in flexible node allocation Jun 16, 2020
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

This PR is mixed with commits from other PRs. Can you fix it?

@teojgo teojgo changed the title [feat] Add a maintenance mode in flexible node allocation [feat] Add 'maint' option for flexible node allocation Jun 16, 2020
@teojgo
Copy link
Contributor Author

teojgo commented Jun 16, 2020

This PR is mixed with commits from other PRs. Can you fix it?

Yes I made a mistake when pushing because it have forked from another branch.

@teojgo teojgo changed the title [feat] Add 'maint' option for flexible node allocation [feat] Add a maintenance option in flexible node allocation Jun 16, 2020
@pep8speaks
Copy link

pep8speaks commented Jun 17, 2020

Hello @teojgo, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2020-06-22 08:32:08 UTC

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2020

Codecov Report

Merging #1374 into master will increase coverage by 0.06%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1374      +/-   ##
==========================================
+ Coverage   91.68%   91.75%   +0.06%     
==========================================
  Files          83       83              
  Lines       12714    12712       -2     
==========================================
+ Hits        11657    11664       +7     
+ Misses       1057     1048       -9     
Impacted Files Coverage Δ
reframe/frontend/cli.py 78.15% <ø> (+0.17%) ⬆️
unittests/test_schedulers.py 93.82% <93.75%> (+1.59%) ⬆️
reframe/core/schedulers/__init__.py 98.83% <100.00%> (ø)
reframe/core/schedulers/local.py 100.00% <100.00%> (ø)
reframe/core/schedulers/slurm.py 57.54% <100.00%> (ø)

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 8bf8e1e...03c923c. Read the comment docs.

@vkarak vkarak changed the title [feat] Add a maintenance option in flexible node allocation [feat] Allow flexible node allocation to select nodes in any state Jun 17, 2020
Copy link
Contributor

@ekouts ekouts left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

I can still see stale changes from the constraint PR. Plus there is another error that exists also in master most probably. If you pass --flex-alloc-nodes without an argument, it will complain, whereas it should just assume idle.

@teojgo
Copy link
Contributor Author

teojgo commented Jun 19, 2020

I can still see stale changes from the constraint PR. Plus there is another error that exists also in master most probably. If you pass --flex-alloc-nodes without an argument, it will complain, whereas it should just assume idle.

But isn't the point of having it to pass an argument. There is no default if the argument is used. Should I put one?

@vkarak
Copy link
Contributor

vkarak commented Jun 19, 2020

@teojgo I think we say that the default is idle and here we imply that passing an argument is optional, which is not the case as it is now.

@teojgo
Copy link
Contributor Author

teojgo commented Jun 19, 2020

Yeah not using the option at all implies idle

@teojgo
Copy link
Contributor Author

teojgo commented Jun 19, 2020

But if you use the option an argument is required

@vkarak
Copy link
Contributor

vkarak commented Jun 19, 2020

Ok, now I get it. But we should still change the documentation of the command line option:

--flex-alloc-nodes=POLICY
blah blah blah

If this option is not specified, the default allocation policy for flexible tests is 'idle'.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

lgtm

@vkarak vkarak merged commit 3a8d026 into reframe-hpc:master Jun 22, 2020
@teojgo teojgo deleted the feat/flex_alloc_nodes_maint branch June 26, 2020 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a maint mode in --flex-alloc-nodes

5 participants