-
Notifications
You must be signed in to change notification settings - Fork 301
Work around Dataproc Py4J weak container references [reduced-it] [fast-ut] [databricks] #15880
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| # Copyright (c) 2026, NVIDIA CORPORATION. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import gc | ||
| import logging | ||
| import weakref | ||
|
|
||
| from spark_init_internal import ( | ||
| _apply_py4j_strong_ref_workaround_if_needed, | ||
| get_spark_i_know_what_i_am_doing, | ||
| ) | ||
|
|
||
|
|
||
| class _Container: | ||
| pass | ||
|
|
||
|
|
||
| def test_py4j_chained_scala_map_get(): | ||
| spark = get_spark_i_know_what_i_am_doing() | ||
| scala_map = spark.conf._jconf.getAll() | ||
| keys = scala_map.keys().iterator() | ||
|
|
||
| assert keys.hasNext() | ||
| key = keys.next() | ||
| assert scala_map.get(key).get() == scala_map.apply(key) | ||
|
|
||
|
|
||
| def test_py4j_weak_container_is_replaced_with_strong_reference(): | ||
| class WeakJavaMember: | ||
| def __init__(self, name, container, target_id, gateway_client): | ||
| self.container = weakref.ref(container) | ||
|
|
||
| assert _apply_py4j_strong_ref_workaround_if_needed(WeakJavaMember) | ||
|
|
||
| container = _Container() | ||
| container_ref = weakref.ref(container) | ||
| member = WeakJavaMember('get', container, 'o1', None) | ||
| del container | ||
| gc.collect() | ||
|
|
||
| assert container_ref() is member.container | ||
|
|
||
|
|
||
| def test_py4j_workaround_is_idempotent(): | ||
| class WeakJavaMember: | ||
| def __init__(self, name, container, target_id, gateway_client): | ||
| self.container = weakref.ref(container) | ||
|
|
||
| assert _apply_py4j_strong_ref_workaround_if_needed(WeakJavaMember) | ||
| patched_init = WeakJavaMember.__init__ | ||
|
|
||
| assert not _apply_py4j_strong_ref_workaround_if_needed(WeakJavaMember) | ||
| assert WeakJavaMember.__init__ is patched_init | ||
|
|
||
|
|
||
| def test_py4j_upstream_strong_container_is_unchanged(): | ||
| class StrongJavaMember: | ||
| def __init__(self, name, container, target_id, gateway_client): | ||
| self.container = container | ||
|
|
||
| original_init = StrongJavaMember.__init__ | ||
|
|
||
| assert not _apply_py4j_strong_ref_workaround_if_needed(StrongJavaMember) | ||
| assert StrongJavaMember.__init__ is original_init | ||
|
|
||
|
|
||
| def test_py4j_probe_failure_skips_workaround(caplog): | ||
| class FailingJavaMember: | ||
| def __init__(self, name, container, target_id, gateway_client): | ||
| raise RuntimeError('probe failed') | ||
|
|
||
| original_init = FailingJavaMember.__init__ | ||
| with caplog.at_level(logging.ERROR): | ||
| assert not _apply_py4j_strong_ref_workaround_if_needed(FailingJavaMember) | ||
|
|
||
| assert FailingJavaMember.__init__ is original_init | ||
| assert 'continuing without the temporary integration-test workaround' in caplog.text | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,12 +12,14 @@ | |
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import functools | ||
| import logging | ||
| import os | ||
| import pytest | ||
| import re | ||
| import stat | ||
| import traceback | ||
| import weakref | ||
|
|
||
| logging.basicConfig( | ||
| format="%(asctime)s %(levelname)-8s %(message)s", | ||
|
|
@@ -55,6 +57,60 @@ def conf_for_env(env_name): | |
| # does not raise NameError if session startup fails before assignment. | ||
| _spark = None | ||
|
|
||
| _PY4J_STRONG_REF_WORKAROUND_MARKER = '_spark_rapids_strong_ref_workaround' | ||
|
|
||
|
|
||
| def _java_member_uses_weak_container(java_member_class): | ||
| class _ProbeContainer: | ||
| pass | ||
|
|
||
| class _ProbeGatewayProperty: | ||
| pool = None | ||
|
|
||
| class _ProbeGatewayClient: | ||
| gateway_property = _ProbeGatewayProperty() | ||
| converters = () | ||
|
|
||
| container = _ProbeContainer() | ||
| member = java_member_class( | ||
| '_spark_rapids_container_lifetime_probe', container, 'o0', _ProbeGatewayClient()) | ||
| return isinstance(member.container, weakref.ReferenceType) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probe constructs
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, updated. |
||
|
|
||
|
|
||
| def _apply_py4j_strong_ref_workaround_if_needed(java_member_class=None): | ||
| """Restore upstream JavaMember container lifetime semantics in this IT process.""" | ||
| if java_member_class is None: | ||
| from py4j.java_gateway import JavaMember | ||
| java_member_class = JavaMember | ||
|
|
||
| original_init = java_member_class.__init__ | ||
| if getattr(original_init, _PY4J_STRONG_REF_WORKAROUND_MARKER, False): | ||
| return False | ||
| try: | ||
| uses_weak_container = _java_member_uses_weak_container(java_member_class) | ||
| except Exception: | ||
| logging.exception( | ||
| "Could not probe Py4J JavaMember container lifetime; continuing without the " | ||
| "temporary integration-test workaround") | ||
| return False | ||
| if not uses_weak_container: | ||
| return False | ||
|
|
||
| @functools.wraps(original_init) | ||
| def strong_ref_init(self, name, container, target_id, gateway_client): | ||
| original_init(self, name, container, target_id, gateway_client) | ||
| if isinstance(self.container, weakref.ReferenceType): | ||
| self.container = container | ||
|
|
||
| setattr(strong_ref_init, _PY4J_STRONG_REF_WORKAROUND_MARKER, True) | ||
| java_member_class.__init__ = strong_ref_init | ||
| logging.warning( | ||
| "Detected weak Py4J JavaMember container references; applying the temporary " | ||
| "integration-test strong-reference workaround for the missing-target failures in " | ||
| "https://github.com/NVIDIA/cudf-spark/issues/15805") | ||
| return True | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
|
|
||
|
|
||
| def findspark_init(): | ||
| import findspark | ||
| findspark.init() | ||
|
|
@@ -120,6 +176,15 @@ def pytest_sessionstart(session): | |
| import pyspark | ||
| from py4j.java_gateway import java_import | ||
|
|
||
| # Dataproc 2.2.86 can delete temporary JVM targets before chained Py4J calls complete. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a process-wide monkeypatch of the IT harness, not a Dataproc/Py4J platform fix. That boundary is fine, but a green Linux/Databricks IT run should not be read as fixing user
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
| # Keep this before SparkSession creation. The behavior probe makes it a no-op for upstream Py4J. | ||
| try: | ||
| _apply_py4j_strong_ref_workaround_if_needed() | ||
| except Exception: | ||
| logging.exception( | ||
| "Could not initialize the temporary Py4J integration-test workaround; " | ||
| "continuing without it") | ||
|
|
||
| # Force the RapidsPlugin to be enabled, so it blows up if the classpath is not set properly | ||
| # DO NOT SET ANY OTHER CONFIGS HERE!!! | ||
| # due to bugs in pyspark/pytest it looks like any configs set here | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Spark-backed regression invokes the chained JVM map access only through the shared default session, so it can pass without confirming that the same call remains functional across the harness's CPU and GPU session configurations. Use the repository's CPU/GPU comparison or fallback assertion path for this integration test.
Rule Used: Integration tests must verify GPU execution using ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regression targets driver-side Py4J
JavaMemberlifetime, not Spark query execution.with_cpu_sessionandwith_gpu_sessionshare the same SparkSession and gateway and only toggle RAPIDS SQL configuration, so they do not exercise a different lifetime path. The GPU/fallback assertion helpers require a DataFrame and execution plan, while this test intentionally exercises the exactscala_map.get(key).get()failure shape directly after the workaround is installed before SparkSession creation. Importingspark_sessionto use those helpers also runs the vulnerable_from_scala_mappath during module initialization. A CPU/GPU split is therefore not applicable here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right — this regression is specifically about driver-side Py4J object lifetime, and the CPU/GPU helpers would not provide additional coverage because they reuse the same SparkSession and gateway while only changing RAPIDS SQL configuration. They also introduce the unrelated DataFrame/plan path and can trigger the vulnerable initialization earlier. The direct chained-call test is the appropriate coverage here, so the CPU/GPU session coverage request does not apply.