[SPARK-58192][CORE][FOLLOWUP] Preserve exact task CPU amounts in PySpark - #57810
[SPARK-58192][CORE][FOLLOWUP] Preserve exact task CPU amounts in PySpark#57810cloud-fan wants to merge 3 commits into
Conversation
Clarify the supported CPU amount range and avoid describing PySpark's floating-point CPU amount as exact. Also clean up wording in comments introduced by the fractional CPU change.
szehon-ho
left a comment
There was a problem hiding this comment.
Docs-only followup, and the range claim it adds checks out against CpuAmount and every enforcement site, so this looks good to merge. A few polish items -- two inline, plus these that touch files outside the diff:
-
python/pyspark/tests/test_taskcontext.py:355and:386still describe the PythoncpuAmount()as exposing "the exact" amount ("the exact cpu amount is available", "cpuAmount() exposes the exact fractional amount"). Since the PySpark hunk here is specifically about not calling that accessor exact, those two docstrings are the same claim in another place. The assertions themselves are fine --0.5and2.0are binary-exact, so nothing wrong is being enshrined. -
docs/configuration.md(thespark.task.cpusentry) documents only that fractional values are allowed. The config parser (CPUS_PER_TASK) rejects out-of-range values and also rejects more than 9 decimal places outright ("supports at most 9 decimal places") -- so the config path errors where theDoublerequest API rounds. That makes the constraint a user is most likely to hit the one with no user-facing documentation, while this PR clarifies the scaladoc most users never read. Worth mirroring the range and the precision limit there.
One question: TaskContext.cpus() is @deprecated(..., "4.3.0") on the Scala side, but the PySpark TaskContext.cpus() reworded here has no .. deprecated:: 4.3.0 marker, so Python users get no signal to migrate to cpuAmount(). That predates this PR rather than being introduced by it, but this PR is editing exactly that docstring -- was leaving it out deliberate?
| * pool rather than an addressable resource, so any amount from 1e-9 through | ||
| * Int.MaxValue is valid, e.g. 1.5; the cpus amount is rounded to the nearest 1e-9, | ||
| * so precision beyond 9 decimal places is not preserved. |
There was a problem hiding this comment.
Both sibling docs qualify this bound with "after rounding" -- TaskResourceRequests.cpus just below, and the PySpark TaskResourceRequests.cpus docstring ("valid from 1e-9 to 2147483647 after rounding"). The qualifier matters here: the check is CpuAmount.isInRange(CpuAmount.normalize(...)), so the bound applies to the value after HALF_UP rounding to scale 9, and a Double half a step outside the range is accepted -- 5e-10 is the example the TaskResourceRequests scaladoc itself gives. The next clause does mention the rounding, but a reader taking "from 1e-9 through Int.MaxValue" at face value gets a slightly narrower range than what the code enforces. Adding "after rounding" would make all three read the same.
| def cpuAmount(self) -> float: | ||
| """ | ||
| The exact amount of CPUs allocated to the task. This can be fractional when | ||
| The amount of CPUs allocated to the task. This can be fractional when |
There was a problem hiding this comment.
The PR description's rationale is that converting the exact decimal amount to a Python float can lose precision, and that the docs should state that constraint. Dropping "exact" makes this not-wrong, but it doesn't tell the reader anything about the approximation. The JVM side leans the other way -- TaskContext.cpuAmount() documents a BigDecimal, and cpus() points at it "for the exact value" -- so someone comparing the two would reasonably assume parity. Maybe say the value is returned as a float and can differ slightly from the exact decimal amount?
| @@ -278,7 +277,7 @@ def cpus(self) -> int: | |||
|
|
|||
| def cpuAmount(self) -> float: | |||
There was a problem hiding this comment.
Consulted the AI, and Python also has a built-in type decimal.Decimal that is similar to Java's BigDecimal, I think I made a wrong decision to use float here ...
| allocation is fractional. Use :meth:`TaskContext.cpuAmount` to get the exact, | ||
| possibly fractional, amount. | ||
| CPUs allocated to the task, rounded up to a whole number when the allocation is | ||
| fractional. Use :meth:`TaskContext.cpuAmount` to get the possibly fractional amount. |
There was a problem hiding this comment.
is it still worth removing "exact"? but either version is fine to me
pan3793
left a comment
There was a problem hiding this comment.
LGTM, but the PR title and description should be updated to reflect the final state of the change before merging.
|
the k8s test error is unrelated, thanks for review, merging to master/4.x/4.3! |
### What changes were proposed in this pull request? Follow-up to #57332. This PR changes PySpark `TaskContext.cpuAmount()` to return `decimal.Decimal` instead of `float`, preserving the exact task CPU amount sent by the JVM. It also updates the corresponding worker message type and tests, clarifies the valid fractional CPU range after rounding, and fixes wording issues in comments added by the original change. ### Why are the changes needed? Converting the exact decimal CPU amount to a Python `float` can lose precision. Returning `decimal.Decimal` keeps the Python API consistent with the exact `BigDecimal` representation used by the JVM. ### Does this PR introduce _any_ user-facing change? Yes. In the unreleased fractional CPU API, PySpark `TaskContext.cpuAmount()` now returns `decimal.Decimal` instead of `float`. ### How was this patch tested? Updated `pyspark.tests.test_taskcontext` to verify integer and 9-decimal-place fractional CPU amounts as `decimal.Decimal` values. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: OpenAI Codex (GPT-5) Closes #57810 from cloud-fan/SPARK-58192-followup. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit da5f016) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
### What changes were proposed in this pull request? Follow-up to #57332. This PR changes PySpark `TaskContext.cpuAmount()` to return `decimal.Decimal` instead of `float`, preserving the exact task CPU amount sent by the JVM. It also updates the corresponding worker message type and tests, clarifies the valid fractional CPU range after rounding, and fixes wording issues in comments added by the original change. ### Why are the changes needed? Converting the exact decimal CPU amount to a Python `float` can lose precision. Returning `decimal.Decimal` keeps the Python API consistent with the exact `BigDecimal` representation used by the JVM. ### Does this PR introduce _any_ user-facing change? Yes. In the unreleased fractional CPU API, PySpark `TaskContext.cpuAmount()` now returns `decimal.Decimal` instead of `float`. ### How was this patch tested? Updated `pyspark.tests.test_taskcontext` to verify integer and 9-decimal-place fractional CPU amounts as `decimal.Decimal` values. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: OpenAI Codex (GPT-5) Closes #57810 from cloud-fan/SPARK-58192-followup. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit da5f016) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
Follow-up to #57332.
This PR changes PySpark
TaskContext.cpuAmount()to returndecimal.Decimalinstead offloat, preserving the exact task CPU amount sent by the JVM. It also updates the correspondingworker message type and tests, clarifies the valid fractional CPU range after rounding, and fixes
wording issues in comments added by the original change.
Why are the changes needed?
Converting the exact decimal CPU amount to a Python
floatcan lose precision. Returningdecimal.Decimalkeeps the Python API consistent with the exactBigDecimalrepresentationused by the JVM.
Does this PR introduce any user-facing change?
Yes. In the unreleased fractional CPU API, PySpark
TaskContext.cpuAmount()now returnsdecimal.Decimalinstead offloat.How was this patch tested?
Updated
pyspark.tests.test_taskcontextto verify integer and 9-decimal-place fractional CPUamounts as
decimal.Decimalvalues.Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex (GPT-5)