gemini-code-assist[bot] commented on code in PR #36852: URL: https://github.com/apache/beam/pull/36852#discussion_r2543470212
########## sdks/python/apache_beam/typehints/native_type_compatibility.py: ########## @@ -342,6 +342,7 @@ def convert_to_beam_type(typ): # Unwrap Python 3.12 `type` aliases (TypeAliasType) to their underlying value. # This ensures Beam sees the actual aliased type (e.g., tuple[int, ...]). + import sys Review Comment:  The `sys` module is already imported at the top of this file (line 25). This local import is redundant and should be removed to adhere to PEP 8 guidelines, which recommend placing all imports at the top of the module. ########## sdks/python/conftest.py: ########## @@ -101,56 +111,88 @@ def configure_beam_rpc_timeouts(): print("Successfully configured Beam RPC timeouts") +def _running_in_ci(): + """Returns True if running in a CI environment.""" + return ( + os.getenv('GITHUB_ACTIONS') == 'true' or + os.getenv('CI') == 'true' or + os.getenv('CONTINUOUS_INTEGRATION') == 'true' + ) + + +def _should_enable_test_cleanup(config): + """Returns True if expensive cleanup operations should run.""" + if config.getoption('--disable-test-cleanup'): + result = False + reason = "disabled via --disable-test-cleanup" + elif config.getoption('--enable-test-cleanup'): + result = True + reason = "enabled via --enable-test-cleanup" + else: + if _running_in_ci(): + result = True + reason = "CI detected" + else: + result = False + reason = "local development" + + # Log once per session + if not hasattr(config, '_cleanup_decision_logged'): + print(f"\n[Test Cleanup] Enabled: {result} ({reason})") + config._cleanup_decision_logged = True + + return result + + @pytest.fixture(autouse=True) -def ensure_clean_state(): +def ensure_clean_state(request): """ - Ensure clean state before each test - to prevent cross-test contamination. + Ensures clean state between tests to prevent contamination. + + Expensive operations (sleeps, extra GC) only run in CI or when + explicitly enabled to keep local tests fast. """ import gc import threading import time - # Force garbage collection to clean up any lingering resources - gc.collect() + enable_cleanup = _should_enable_test_cleanup(request.config) + + if enable_cleanup: + gc.collect() - # Log active thread count for debugging thread_count = threading.active_count() - if thread_count > 50: # Increased threshold since we see 104 threads + if thread_count > 50: print(f"Warning: {thread_count} active threads detected before test") - - # Force a brief pause to let threads settle - time.sleep(0.5) - gc.collect() + if enable_cleanup: + time.sleep(0.5) + gc.collect() yield - # Enhanced cleanup after test try: - # Force more aggressive cleanup - gc.collect() - - # Brief pause to let any async operations complete - time.sleep(0.1) - - # Additional garbage collection - gc.collect() + if enable_cleanup: + gc.collect() + time.sleep(0.1) + gc.collect() except Exception as e: print(f"Warning: Cleanup error: {e}") @pytest.fixture(autouse=True) -def enhance_mock_stability(): - """Enhance mock stability in DinD environment.""" +def enhance_mock_stability(request): + """Improves mock stability in DinD environment.""" import time Review Comment:  According to PEP 8, imports should be at the top of the file. The `time` module is part of the standard library. It would be better to move this import to the top of the file (with other standard library imports like `os` and `sys`) to improve code readability and adhere to standard Python style. This also avoids re-importing the module. ########## sdks/python/conftest.py: ########## @@ -101,56 +111,88 @@ def configure_beam_rpc_timeouts(): print("Successfully configured Beam RPC timeouts") +def _running_in_ci(): + """Returns True if running in a CI environment.""" + return ( + os.getenv('GITHUB_ACTIONS') == 'true' or + os.getenv('CI') == 'true' or + os.getenv('CONTINUOUS_INTEGRATION') == 'true' + ) + + +def _should_enable_test_cleanup(config): + """Returns True if expensive cleanup operations should run.""" + if config.getoption('--disable-test-cleanup'): + result = False + reason = "disabled via --disable-test-cleanup" + elif config.getoption('--enable-test-cleanup'): + result = True + reason = "enabled via --enable-test-cleanup" + else: + if _running_in_ci(): + result = True + reason = "CI detected" + else: + result = False + reason = "local development" + + # Log once per session + if not hasattr(config, '_cleanup_decision_logged'): + print(f"\n[Test Cleanup] Enabled: {result} ({reason})") + config._cleanup_decision_logged = True + + return result + + @pytest.fixture(autouse=True) -def ensure_clean_state(): +def ensure_clean_state(request): """ - Ensure clean state before each test - to prevent cross-test contamination. + Ensures clean state between tests to prevent contamination. + + Expensive operations (sleeps, extra GC) only run in CI or when + explicitly enabled to keep local tests fast. """ import gc import threading import time Review Comment:  According to PEP 8, imports should be at the top of the file. The `gc`, `threading`, and `time` modules are part of the standard library. It would be better to move these imports to the top of the file (with other standard library imports like `os` and `sys`) to improve code readability and adhere to standard Python style. ########## sdks/python/conftest.py: ########## @@ -101,56 +111,88 @@ def configure_beam_rpc_timeouts(): print("Successfully configured Beam RPC timeouts") +def _running_in_ci(): + """Returns True if running in a CI environment.""" + return ( + os.getenv('GITHUB_ACTIONS') == 'true' or + os.getenv('CI') == 'true' or + os.getenv('CONTINUOUS_INTEGRATION') == 'true' + ) + + +def _should_enable_test_cleanup(config): + """Returns True if expensive cleanup operations should run.""" + if config.getoption('--disable-test-cleanup'): + result = False + reason = "disabled via --disable-test-cleanup" + elif config.getoption('--enable-test-cleanup'): + result = True + reason = "enabled via --enable-test-cleanup" + else: + if _running_in_ci(): + result = True + reason = "CI detected" + else: + result = False + reason = "local development" + + # Log once per session + if not hasattr(config, '_cleanup_decision_logged'): + print(f"\n[Test Cleanup] Enabled: {result} ({reason})") + config._cleanup_decision_logged = True + + return result Review Comment:  The `_should_enable_test_cleanup` function is called by both `ensure_clean_state` and `enhance_mock_stability` fixtures for every test. Since the result of this function is the same for the entire test session, it can be cached on the `config` object to avoid re-computation. This will provide a small performance improvement, which aligns with the goal of this pull request. ```python def _should_enable_test_cleanup(config): """Returns True if expensive cleanup operations should run.""" if hasattr(config, '_should_enable_test_cleanup_result'): return config._should_enable_test_cleanup_result if config.getoption('--disable-test-cleanup'): result = False reason = "disabled via --disable-test-cleanup" elif config.getoption('--enable-test-cleanup'): result = True reason = "enabled via --enable-test-cleanup" else: if _running_in_ci(): result = True reason = "CI detected" else: result = False reason = "local development" # Log once per session if not hasattr(config, '_cleanup_decision_logged'): print(f"\n[Test Cleanup] Enabled: {result} ({reason})") config._cleanup_decision_logged = True config._should_enable_test_cleanup_result = result return result ``` -- 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]
