pan3793 commented on code in PR #57332:
URL: https://github.com/apache/spark/pull/57332#discussion_r3614315806
##########
core/src/main/scala/org/apache/spark/resource/TaskResourceRequest.scala:
##########
@@ -28,17 +28,28 @@ import org.apache.spark.annotation.{Since, Stable}
*
* @param resourceName Resource name
* @param amount Amount requesting as a Double to support fractional resource
requests.
- * Valid values are less than or equal to 1.0 or whole numbers.
This essentially
- * lets you configure X number of tasks to run on a single
resource,
- * ie amount equals 0.5 translates into 2 tasks per resource
address.
+ * For custom resources, valid values are less than or equal to
1.0 or whole
+ * numbers, since a task's amount must map onto discrete
resource addresses -
+ * ie amount equals 0.5 translates into 2 tasks per resource
address. CPUs
+ * (resource name "cpus") are a plain quantity drawn from the
executor's core
+ * pool rather than an addressable resource, so any amount of at
least 1e-9 is
+ * valid, e.g. 1.5.
*/
@Stable
@Since("3.1.0")
class TaskResourceRequest(val resourceName: String, val amount: Double)
extends Serializable {
- assert(amount <= 1.0 || amount % 1 == 0,
- s"The resource amount ${amount} must be either <= 1.0, or a whole number.")
+ if (resourceName == ResourceProfile.CPUS) {
+ // A positive amount below half of the internal accounting scale (1e-9)
would silently
+ // round to zero cpus downstream; reject it here where the original value
is still visible.
+ assert(!amount.isNaN && !amount.isInfinity &&
+ CpuAmount.normalize(BigDecimal(amount.toString)).signum > 0,
Review Comment:
Thanks for catching this — I dug in and I'd like to argue for keeping the
current rounding behavior (with a doc clarification) rather than adding
rejection, because the config path and this API take fundamentally different
input types:
- `spark.task.cpus` is parsed from an **exact decimal string**, so rejecting
`scale > 9` is safe: a user writes `0.3`, never `0.30000000000000004`.
- `TaskResourceRequests.cpus(...)` takes a **`Double`**, which carries
binary-representation noise. Porting the config's "reject excess precision"
rule here would reject ordinary computed values:
| call | `amount.toString` | stripped scale | today (`HALF_UP` @ `1e-9`) |
|---|---|---|---|
| `cpus(0.3)` | `0.3` | 1 | `0.300000000` |
| `cpus(0.1 + 0.2)` | `0.30000000000000004` | 17 | `0.300000000` |
| `cpus(1.0/3)` | `0.3333333333333333` | 16 | `0.333333333` |
| `cpus(1.0000000004)` | `1.0000000004` | 10 | `1.000000000` |
So `cpus(0.1 + 0.2)` and `cpus(1.0/3)` would be rejected even though the
intent is clearly ~`0.3` and ~`0.333`. Rounding to `1e-9` is exactly what
cleans that noise up, so for a `Double` input rounding is the more
forgiving/correct behavior — the asymmetry with the config path is a
consequence of exact-string vs. lossy-double inputs, not an oversight.
On the specific harm (`cpus(1.0000000004)` → `1.0` → 10 slots instead of 9):
it needs the argument to land within ~`5e-10` of a slot boundary, which isn't a
realistic configuration. The "round up, never down" alternative avoids
under-allocation but trades it for surprising values like `0.3 → 0.300000001`.
The only way to make strict rejection coherent is to accept exact decimals,
i.e. re-type `TaskResourceRequest.amount` from `Double` to `BigDecimal`. That's
a `@Stable` / `@Since("3.1.0")` public field shared by every resource type (the
gpu/fpga fractional logic, the Java/Python mirrors, MiMa), so I'd prefer not to
do that.
Proposed resolution: keep the `Double` API + `HALF_UP` normalization, and
clarify in the scaladoc that a programmatic amount is rounded to the nearest
`1e-9`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
