HappenLee commented on code in PR #69255:
URL: https://github.com/apache/doris/pull/69255#discussion_r4374552510
##########
be/src/udf/python/python_server.py:
##########
@@ -1113,39 +1024,7 @@ def _get_or_import_module(self, location: str,
full_module_name: str) -> Any:
f"server-critical modules."
)
- cache_key = location
-
- # Use a per-module lock to prevent race conditions during import
- import_lock = ModuleUDFLoader._get_import_lock(full_module_name)
-
- with import_lock:
- # Fast path: check cache first
- if cache_key in ModuleUDFLoader._module_cache:
- cached_module = ModuleUDFLoader._module_cache[cache_key]
- if cached_module is not None and (
- hasattr(cached_module, "__file__")
- or hasattr(cached_module, "__path__")
- ):
- return cached_module
- else:
- del ModuleUDFLoader._module_cache[cache_key]
-
- self._clear_modules_from_sys(full_module_name)
-
- with temporary_sys_path(location):
- try:
- module = importlib.import_module(full_module_name)
- ModuleUDFLoader._module_cache[cache_key] = module
- # Evict from sys.modules so future imports from a
- # different location are not poisoned by this one.
- self._clear_modules_from_sys(full_module_name)
- return module
- except Exception:
- # Clean up any partially-imported modules
- self._clear_modules_from_sys(full_module_name)
- if cache_key in ModuleUDFLoader._module_cache:
- del ModuleUDFLoader._module_cache[cache_key]
- raise
+ return importlib.import_module(full_module_name)
Review Comment:
Following up on the naming-conflict concern: this is a reproducible
compatibility regression introduced by this PR, rather than just a packaging
convention users already had to follow.
For example, upload a package containing `types.py`:
```python
def evaluate(x):
return x + 7
```
and configure `symbol = "types.evaluate"`. An isolated probe using the
actual loader definitions from the merge base (`6e5a8929fbab`) and current head
(`eee46a1dc20a`) gives:
- Before this PR: `evaluate(5)` returns `12`.
- Current head: `ImportError: Failed to load single-file UDF
'types.evaluate': Function 'evaluate' not found in module 'types'`.
The Python server already has stdlib `types` in `sys.modules` through its
startup dependencies. Adding the UDF directory to `sys.path` does not override
that cached module. The old loader evicted the conflicting module key before
importing; the new direct `importlib.import_module()` returns the server's
cached module. A dedicated process removes collisions between different UDFs,
but still contains the server and its preloaded dependencies. `types` is not in
the existing forbidden-name set, so asking users to avoid it leaves a
previously working package broken.
Please resolve the entry-point module against the supplied package while
preserving the native caching benefits. One implementation direction is a
function-id-specific module namespace, e.g. `_doris_udf_12345.types`, leaving
stdlib `types` intact. That approach needs compatibility coverage for package
absolute/relative imports, lazy imports, concurrent first loading, and UDAF
custom-state pickle across workers with stable module names. The example above
should also become a regression test. Merely extending the module-name
blacklist or repeatedly removing server modules would leave the underlying
namespace problem.
Validation here was an isolated loader comparison; a full Doris-cluster
reproduction was not run.
--
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]
