[SPARK-50421][CORE] Fix executor related memory config incorrect when multiple resource profiles worked by zjuwangg · Pull Request #48963 · apache/spark

zjuwangg · 2024-11-26T03:44:39Z

What changes were proposed in this pull request?

Reset the executor's env memory related config when resource profile is not as the default resource profile!

Why are the changes needed?

When multiple resource profile exists in the same spark application, now the executor's memory related config is not override by resource profile's memory size, which will cause maxOffHeap in UnifiedMemoryManager is not correct.
See https://issues.apache.org/jira/browse/SPARK-50421 for more details

Does this PR introduce any user-facing change?

No

How was this patch tested?

Tests in our inner spark version and jobs.

Was this patch authored or co-authored using generative AI tooling?

No

zjuwangg · 2024-11-26T03:47:20Z

@tgravescs Would you help to review this commit?

…rofile worked

tgravescs · 2024-12-02T17:13:04Z

+            case (ResourceProfile.OFFHEAP_MEM, request) =>
+              driverConf.set(MEMORY_OFFHEAP_SIZE.key, request.amount.toString + "m")
+              if (request.amount > 0) {
+                driverConf.set(MEMORY_OFFHEAP_ENABLED.key, "true")


this is questionable to me, we haven't supported setting confs yet so I would expect it to pick up the default config for this. I'd rather see a separate issue if we want to revisit this behavior.

Thank you very much for this PR, we just encountered this problem.
Is the logic of resetting the offheap configuration better this way?

case (ResourceProfile.OFFHEAP_MEM, request) if request.amount > 0 => driverConf.set(MEMORY_OFFHEAP_SIZE.key, request.amount.toString + "m") driverConf.set(MEMORY_OFFHEAP_ENABLED.key, "true")

this is questionable to me, we haven't supported setting confs yet so I would expect it to pick up the default config for this. I'd rather see a separate issue if we want to revisit this behavior.

Make sense to me. I'll address it later, how about introducing a config to control whether to control offheap enabled or not?

Thank you very much for this PR, we just encountered this problem. Is the logic of resetting the offheap configuration better this way?

case (ResourceProfile.OFFHEAP_MEM, request) if request.amount > 0 => driverConf.set(MEMORY_OFFHEAP_SIZE.key, request.amount.toString + "m") driverConf.set(MEMORY_OFFHEAP_ENABLED.key, "true")

@xumanbu
I think this is incorrect. Imagine such a scenario where default resource profile's offheap is 512M, and resource profile with id 2 's offheap is 0, in such case the offheap config will be incorrect!

zjuwangg · 2024-12-03T12:59:27Z

@tgravescs I just updated the commit as the comments. Please help review when you have free time.

tgravescs · 2024-12-03T16:32:13Z

what all testing have you done with this? It would be nice to have a integration test with it but I know this would be difficult.

zjuwangg · 2024-12-04T10:34:57Z

what all testing have you done with this? It would be nice to have a integration test with it but I know this would be difficult.

I have test this in our inner spark version and UnifiedMemoryManager worked as expected. I just tried to add a integration test but found it's hard since there no easy way to mock driver and executor to verify the code.

tgravescs

+1 looks fine to me.

zjuwangg · 2024-12-05T04:57:42Z

+1 looks fine to me.

Thanks! Would you help merge it?
I think this also should be included in spark 3.5.4

dongjoon-hyun

+1, LGTM. Thank you, @zjuwangg , @tgravescs , @xumanbu .

Merged to master.

cc @LuciferYang as a release manager of Apache Spark 3.5.4

dongjoon-hyun · 2024-12-05T16:27:18Z

Due to the Structured Log change, this patch is not applicable to branch-3.5.

Please make a backporting PR to branch-3.5, @zjuwangg .

dongjoon-hyun · 2024-12-05T16:30:52Z

BTW, I added you to Apache Spark Contributor group in JIRA and assigned SPARK-50421 to you, @zjuwangg .

Welcome to the Apache Spark community and thank you again.

zjuwangg · 2024-12-06T08:16:14Z

Due to the Structured Log change, this patch is not applicable to branch-3.5.

Please make a backporting PR to branch-3.5, @zjuwangg .

I'll make a backporting PR to branch-3.5 soon.

zjuwangg · 2024-12-06T08:32:30Z

Due to the Structured Log change, this patch is not applicable to branch-3.5.
Please make a backporting PR to branch-3.5, @zjuwangg .

I'll make a backporting PR to branch-3.5 soon.

Backporting PR: #49090

… when multiple resource profiles worked ### What changes were proposed in this pull request? Reset the executor's env memory related config when resource profile is not as the default resource profile! ### Why are the changes needed? When multiple resource profile exists in the same spark application, now the executor's memory related config is not override by resource profile's memory size, which will cause maxOffHeap in `UnifiedMemoryManager` is not correct. See https://issues.apache.org/jira/browse/SPARK-50421 for more details ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Tests in our inner spark version and jobs. ### Was this patch authored or co-authored using generative AI tooling? No This is a backporting from #48963 to branch 3.5 Closes #49090 from zjuwangg/m35_fixConfig. Authored-by: Terry Wang <zjuwangg@foxmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

mridulm · 2024-12-15T21:37:22Z

+                log"${MDC(LogKeys.EXECUTOR_MEMORY_OVERHEAD_SIZE, request)}")
+            case (ResourceProfile.CORES, request) =>
+              driverConf.set(EXECUTOR_CORES.key, request.amount.toString)
+              logInfo(log"Set executor cores to ${MDC(LogKeys.NUM_EXECUTOR_CORES, request)}")


Dont we need to do this for PYSPARK_MEM as well ? (from ResourceProfile.allSupportedExecutorResources)

PYSPARK_MEM seems no actual use during task execute like EXECUTOR_MEMORY_OVERHEAD, but I think it's nice to have it here to keep consistent.

github-actions bot added the CORE label Nov 26, 2024

zjuwangg added 2 commits November 26, 2024 15:27

fix executor related memory config incorrect when multiple resource p…

9efe1e8

…rofile worked

fix byte unit is incorrect

1e41213

zjuwangg force-pushed the m_fixConfig branch from d28c67f to 1e41213 Compare November 26, 2024 07:27

zjuwangg added 2 commits November 26, 2024 16:43

support set executor cores

c32682a

only set offheap enabled if necessary

acaf456

zjuwangg changed the title ~~[SPARK-50421][wip]fix executor related memory config incorrect when multiple resource profile worked~~ [SPARK-50421]fix executor related memory config incorrect when multiple resource profile worked Nov 26, 2024

fix log code style not invalid

417a5b8

tgravescs reviewed Dec 2, 2024

View reviewed changes

address comments

776b006

zjuwangg changed the title ~~[SPARK-50421]fix executor related memory config incorrect when multiple resource profile worked~~ [SPARK-50421]Fix executor related memory config incorrect when multiple resource profile worked Dec 3, 2024

fix compile error

fabb530

fix test error

6ce3e0a

tgravescs reviewed Dec 4, 2024

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-50421]Fix executor related memory config incorrect when multiple resource profile worked~~ [SPARK-50421][CORE] Fix executor related memory config incorrect when multiple resource profiles worked Dec 5, 2024

dongjoon-hyun approved these changes Dec 5, 2024

View reviewed changes

dongjoon-hyun closed this in faa2b04 Dec 5, 2024

zjuwangg mentioned this pull request Dec 6, 2024

[SPARK-50421][CORE][3.5] Fix executor related memory config incorrect when multiple resource profiles worked #49090

Closed

zjuwangg deleted the m_fixConfig branch December 6, 2024 08:33

mridulm reviewed Dec 15, 2024

View reviewed changes

jackylee-ch mentioned this pull request Apr 7, 2025

[CORE] Decrease offheap memory size in resource profile for whole stage fallback case apache/gluten#8911

Merged

Conversation

zjuwangg commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

zjuwangg commented Nov 26, 2024

Uh oh!

tgravescs Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

xumanbu Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

zjuwangg Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zjuwangg Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zjuwangg commented Dec 3, 2024

Uh oh!

tgravescs commented Dec 3, 2024

Uh oh!

zjuwangg commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgravescs left a comment

Choose a reason for hiding this comment

Uh oh!

zjuwangg commented Dec 5, 2024

Uh oh!

dongjoon-hyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Dec 5, 2024

Uh oh!

dongjoon-hyun commented Dec 5, 2024

Uh oh!

zjuwangg commented Dec 6, 2024

Uh oh!

zjuwangg commented Dec 6, 2024

Uh oh!

mridulm Dec 15, 2024

Choose a reason for hiding this comment

Uh oh!

zjuwangg Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zjuwangg commented Nov 26, 2024 •

edited

Loading

zjuwangg Dec 3, 2024 •

edited

Loading

zjuwangg Dec 3, 2024 •

edited

Loading

zjuwangg commented Dec 4, 2024 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

zjuwangg Dec 16, 2024 •

edited

Loading