Re: Review Request 44365: Added flag to specify available Nvidia GPUs on an agent's command line.

2016-03-29 Thread Kevin Klues 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44365/
---

(Updated March 30, 2016, 12:19 a.m.)


Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.


Bugs: MESOS-4864
https://issues.apache.org/jira/browse/MESOS-4864


Repository: mesos


Description
---

Added flag to specify available Nvidia GPUs on an agent's command line.


Diffs (updated)
-

  docs/configuration.md 6fc18754c88df16162d83a31e0ecede324bec971 
  src/slave/flags.hpp 9ee7f349cc2bb3fa76141645f4a06fad57664367 
  src/slave/flags.cpp 0c13ab6eb3bc8754da11104466935062cf20ed87 

Diff: https://reviews.apache.org/r/44365/diff/


Testing
---

./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,string
Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,string': 
Expecting all elements to be unsigned integers

./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2.0,3   
Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,3.0': 
Expecting all elements to be unsigned integers

./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,3
SUCCESS


Thanks,

Kevin Klues

Re: Review Request 44365: Added flag to specify available Nvidia GPUs on an agent's command line.

2016-03-29 Thread Kevin Klues 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44365/
---

(Updated March 30, 2016, 12:16 a.m.)


Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.


Changes
---

Rebased to newest master. Fixed comments as per bmahler's suggestion.


Bugs: MESOS-4864
https://issues.apache.org/jira/browse/MESOS-4864


Repository: mesos


Description
---

Added flag to specify available Nvidia GPUs on an agent's command line.


Diffs (updated)
-

  docs/configuration.md 6fc18754c88df16162d83a31e0ecede324bec971 
  src/slave/flags.hpp 9ee7f349cc2bb3fa76141645f4a06fad57664367 
  src/slave/flags.cpp 0c13ab6eb3bc8754da11104466935062cf20ed87 

Diff: https://reviews.apache.org/r/44365/diff/


Testing
---

./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,string
Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,string': 
Expecting all elements to be unsigned integers

./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2.0,3   
Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,3.0': 
Expecting all elements to be unsigned integers

./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,3
SUCCESS


Thanks,

Kevin Klues

Re: Review Request 44365: Added flag to specify available Nvidia GPUs on an agent's command line.

2016-03-29 Thread Ben Mahler 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44365/#review125985
---


Fix it, then Ship it!





docs/configuration.md (lines 1308 - 1309)


Could we add something to clarify that this is relevant when "gpus" are 
specified within --resources?


- Ben Mahler


On March 14, 2016, 7:38 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44365/
> ---
> 
> (Updated March 14, 2016, 7:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4864
> https://issues.apache.org/jira/browse/MESOS-4864
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added flag to specify available Nvidia GPUs on an agent's command line.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 739d4ff9aeeb1ba70ce11033168d63d37b6ef56b 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 5bacf109bcd9c9eae1b7ec3c32095c72899ebdbd 
> 
> Diff: https://reviews.apache.org/r/44365/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,string
> Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,string': 
> Expecting all elements to be unsigned integers
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2.0,3   
> Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,3.0': 
> Expecting all elements to be unsigned integers
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,3
> SUCCESS
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44365: Added flag to specify available Nvidia GPUs on an agent's command line.

2016-03-14 Thread Kevin Klues 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44365/
---

(Updated March 14, 2016, 7:38 a.m.)


Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.


Changes
---

Addressed all of bmahler's comments.


Bugs: MESOS-4864
https://issues.apache.org/jira/browse/MESOS-4864


Repository: mesos


Description
---

Added flag to specify available Nvidia GPUs on an agent's command line.


Diffs (updated)
-

  docs/configuration.md 739d4ff9aeeb1ba70ce11033168d63d37b6ef56b 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp 5bacf109bcd9c9eae1b7ec3c32095c72899ebdbd 

Diff: https://reviews.apache.org/r/44365/diff/


Testing (updated)
---

./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,string
Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,string': 
Expecting all elements to be unsigned integers

./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2.0,3   
Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,3.0': 
Expecting all elements to be unsigned integers

./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,3
SUCCESS


Thanks,

Kevin Klues

Re: Review Request 44365: Added flag to specify available Nvidia GPUs on an agent's command line.

2016-03-14 Thread Kevin Klues 


> On March 9, 2016, 12:03 a.m., Ben Mahler wrote:
> > src/slave/flags.hpp, line 92
> > 
> >
> > Can you put this in an ifdef?

Done. I also moved it out of the `#ifdef __linux__ `directive.  Although we 
only support gpus on linux at the moment, there's nothing preventing us from 
supporting them on other platforms in the future. This should be treated as a 
generic flag in that respect (i.e. nvidia-smi is also a command that can be run 
on windows).


> On March 9, 2016, 12:03 a.m., Ben Mahler wrote:
> > src/slave/flags.cpp, lines 597-607
> > 
> >
> > -s/nvidia_gpus/nvidia_gpu_devices/
> > -capitalize NVML
> > -s/as a resource/as resources/
> > -maybe put the nvidia-smi comment in a parenthetical?
> > -The third sentence seems to be already expressed in the first? Perhaps 
> > clarify that the isolator will be used only if the `--isolation` flag 
> > contains the right value.
> > -We can probably omit or re-pharse the last sentence since this flag is 
> > omitted from the program if the configure flag is not present.

Done. I also updated the string in `docs/configuration.md`.  In the `.md` file, 
I left the (slightly reworded) last line, since there is no way of `#ifdef`ing 
this flag out in the documentation.


> On March 9, 2016, 12:03 a.m., Ben Mahler wrote:
> > src/slave/flags.cpp, lines 606-607
> > 
> >
> > I was going to suggest we add a validator here, but even better would 
> > be to add a parse specialization to src/common/parse.hpp and just avoid the 
> > need for the extra validation altogether.

Because it is so generic, I decided to add this parser to stout instead of 
mesos.  It should appear as a a dependency of this patch once I push it to 
reviewboard. As part of this, I now set `vector` as the 
return type on the `nvidia_gpu_devices` flag.


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44365/#review122628
---


On March 14, 2016, 7:38 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44365/
> ---
> 
> (Updated March 14, 2016, 7:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4864
> https://issues.apache.org/jira/browse/MESOS-4864
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added flag to specify available Nvidia GPUs on an agent's command line.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 739d4ff9aeeb1ba70ce11033168d63d37b6ef56b 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 5bacf109bcd9c9eae1b7ec3c32095c72899ebdbd 
> 
> Diff: https://reviews.apache.org/r/44365/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,string
> Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,string': 
> Expecting all elements to be unsigned integers
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2.0,3   
> Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,3.0': 
> Expecting all elements to be unsigned integers
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,3
> SUCCESS
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44365: Added flag to specify available Nvidia GPUs on an agent's command line.

2016-03-08 Thread Ben Mahler 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44365/#review122628
---




src/slave/flags.hpp (line 92)


Can you put this in an ifdef?



src/slave/flags.cpp (lines 597 - 607)


-s/nvidia_gpus/nvidia_gpu_devices/
-capitalize NVML
-s/as a resource/as resources/
-maybe put the nvidia-smi comment in a parenthetical?
-The third sentence seems to be already expressed in the first? Perhaps 
clarify that the isolator will be used only if the `--isolation` flag contains 
the right value.
-We can probably omit or re-pharse the last sentence since this flag is 
omitted from the program if the configure flag is not present.



src/slave/flags.cpp (lines 606 - 607)


I was going to suggest we add a validator here, but even better would be to 
add a parse specialization to src/common/parse.hpp and just avoid the need for 
the extra validation altogether.


- Ben Mahler


On March 8, 2016, 10:47 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44365/
> ---
> 
> (Updated March 8, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4864
> https://issues.apache.org/jira/browse/MESOS-4864
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added flag to specify available Nvidia GPUs on an agent's command line.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 
> 
> Diff: https://reviews.apache.org/r/44365/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>