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

flux-mini: add --gpus-per-task option, update flux-mini(1) man page #2409

Merged
merged 6 commits into from Sep 29, 2019

Conversation

@garlick
Copy link
Member

commented Sep 28, 2019

This fixes an error pointed out by @grondo and makes mention of affinity plugin shell options. That raises a question about where we should be documenting shell plugin options. Since they'll be usable from multiple front end tools, this may be the wrong place.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

Documenting options provided by plugins might not be a tractable problem, though I guess these plugins are built in, so it is less likely they will be missing. However, their usage may be overridden by a site plugin, so including them in static manpages might be confusing.

We have a plan for a shell, "validate" mode, maybe we need something similar to generate help message for all plugins loaded in the shell which take options?

For now, maybe just document that these options are provided by shell built-in plugins which may be overridden in some cases?

@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2019

Sounds good. For now, I changed the wording a bit per your suggestion

These options are provided by built-in shell plugins that may be overridden in some cases:

@garlick garlick force-pushed the garlick:mini-manpage branch from f33fda7 to 0d1e625 Sep 29, 2019
garlick and others added 6 commits Sep 28, 2019
Problem: man page mentions -d with --debug,
but -d is not accepted by flux-mini(1).

Drop -d from the man page.

Fixes #2408
Add an explicit option to set the number of GPUs per task in
generated jobspec from flux-mini commands. The resuling "gpu"
resource vertex is always generated at the same level as "core".
Just check that the requested number of gpus appear
in the task slot of the jobspec request.
@garlick garlick force-pushed the garlick:mini-manpage branch from 0d1e625 to 445d3b2 Sep 29, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2019

Added flux-mini update for GPU's originally proposed in PR #2406 (just merged) but backed out.

@garlick garlick changed the title flux-mini(1) man page update flux-mini: add --gpus-per-task option, update flux-mini(1) man page Sep 29, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Sep 29, 2019

Codecov Report

Merging #2409 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2409      +/-   ##
==========================================
+ Coverage   81.13%   81.14%   +0.01%     
==========================================
  Files         225      225              
  Lines       36099    36099              
==========================================
+ Hits        29288    29293       +5     
+ Misses       6811     6806       -5
Impacted Files Coverage Δ
src/modules/job-ingest/worker.c 72.72% <0%> (-1.4%) ⬇️
src/broker/modservice.c 70.67% <0%> (-0.76%) ⬇️
src/cmd/flux-module.c 84.19% <0%> (ø) ⬆️
src/common/libflux/message.c 80.5% <0%> (+0.13%) ⬆️
src/modules/connector-local/local.c 74.27% <0%> (+1.01%) ⬆️
@grondo

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

While we can't do end-to-end testing yet of -g, --gpus-per-task option in flux-core, I did sanity check a version with flux-sched and a fake hwloc XML loaded (from corona) and the option appears to be working: 👍

$ flux kvs get resource.hwloc.by_rank
{"0": {"Package": 2, "NUMANode": 8, "Core": 48, "PU": 96, "GPU": 4, "cpuset": "0-95"}, "1": {"Package": 1, "Core": 4, "PU": 4, "cpuset": "0-3"}}
$ flux mini run -v --label-io -n 2 -g 1 -o gpu-affinity=per-task printenv CUDA_VISIBLE_DEVICES
jobid: 9365562064896
0: 2
1: 3
 $ flux job info 9365562064896 R | jq
{
  "version": 1,
  "execution": {
    "R_lite": [
      {
        "rank": "0",
        "node": "corona11",
        "children": {
          "core": "35,47",
          "gpu": "2-3"
        }
      }
    ]
  }
}
@grondo

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

I think I'll go ahead and just merge this one.

@grondo grondo merged commit 5f41697 into flux-framework:master Sep 29, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch Coverage not affected when comparing 62a92b3...445d3b2
Details
codecov/project 81.14% (+0.01%) compared to 62a92b3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick garlick deleted the garlick:mini-manpage branch Sep 29, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.