Add fractional and subtraction for jobs flag #3554
Conversation
Resolve com-lihaoyi#3482 Allow jobs param to have following formats: 1. An integer indicates the parallel level. 2. A float N followed by character "C" indicates uses (N * all available cores). e.g. "0.5C" uses half of the available cores. 3. "C-" followed by an integer N indicates uses (all available cores - N). e.g. "C-1" leaves 1 core and uses all the other cores.
| def err(detail: String) = new Exception( | ||
| s"Invalid value \"${threadCountRaw.getOrElse("")}\" for flag -j/--jobs: $detail" | ||
| ) | ||
| (threadCountRaw match { | ||
| case None => Success(cores) | ||
| case Some(s"${n}C") => n.toDoubleOption.map(Success(_)) | ||
| .getOrElse(Failure(err("Failed to find a float number before \"C\"."))) | ||
| .map(m => (m * cores).toInt) | ||
| case Some(s"C-${n}") => n.toIntOption.map(Success(_)) | ||
| .getOrElse(Failure(err("Failed to find a int number after \"C-\"."))) | ||
| .map(cores - _) | ||
| case Some(n) => n.toIntOption.map(Success(_)) | ||
| .getOrElse(Failure(err("Failed to find a int number"))) | ||
| }).flatMap { | ||
| case x if x < 0 => Failure(err("Calculated cores to use is a negative number.")) | ||
| case 0 => Success(cores) // use all cores if 0 | ||
| case x => Success(x) | ||
| } | ||
| } |
There was a problem hiding this comment.
Few nits here:
-
Can we extract this into a helper function that we can unit test with various combinations of
detail: Stringandcores: Int? That will give us some confidence at-a-glance that the code is doing what we expect without tedious manual testing -
I don't think the current treatment of "0 = use all cores" makes sense for computed zeros. If someone runs
-j0.4Con a 4 core machine, they probably don't intend to use all 4 cores. Let's treat literal 0 separately from computed numbers, and have computed numbers have a lower limit of 1 core (so0or-1becomes1) -
Lets return an
Either[String, Int]since that's all the information you need later on at the callsite
|
@lihaoyi addressed comments about the code. Where should I put the unit tests? |
|
runner/test/src/mill/runner/ should work i think, run via mill runner.test |
|
@lihaoyi Added unit tests. Seems the auto lint check failed on previous commit again. Is there any command that I can auto format code or show lint errors? |
|
@wb14123 |
|
Hmm I formatted the files. Not sure why it failed again. Does the command above also format the test files? |
|
@wb14123 it should. Could you try merging in latest |
| assertParseErr( | ||
| MillMain.parseThreadCount(Some("0.09C"), 10), | ||
| "Calculated cores to use should be a positive number" | ||
| ) |
There was a problem hiding this comment.
For these computed cases (this one and C-10 and C-11 below), let's always just round up to 1 core minimum. That's probably what users want, so no point erroring out when we can just do it for them
|
Left one more comment, after that I think we're good to merge |
Resolve #3482
Allow jobs param to have following formats: