refactor(pyamber): replace PyFilesystem2 with tempfile and unpin setuptools - #6946
refactor(pyamber): replace PyFilesystem2 with tempfile and unpin setuptools#6946aicam wants to merge 1 commit into
Conversation
…ptools `fs` (PyFilesystem2) is unmaintained -- 2.4.16 (May 2022) is its final release and it calls pkg_resources at import time. setuptools 82.0.0 removed pkg_resources, so `import fs` hard-crashes there, which is the only reason amber/requirements.txt has carried a setuptools pin since apache#4199. ExecutorManager was the only consumer, and fs's TempFS is internally tempfile.mkdtemp + shutil.rmtree, so the swap is behavior-preserving: lazy creation, sys.path append/remove, the never-materialized early return, and the tolerated leak-on-force-kill semantics are unchanged. tempfile.mkdtemp is deliberate over TemporaryDirectory -- the latter attaches a finalizer that would silently clean up abandoned directories. Also calls importlib.invalidate_caches() before importing the freshly written UDF module. The import system caches a directory listing per sys.path entry and refreshes it on mtime change only, so a second UDF written into the same tmp directory within one mtime tick could be invisible to the finder. This reproduces on main as well (four TestUpdateExecutor cases fail locally with fs installed); it is fixed here because the rewritten write-then-import path is where it lives. Dropping fs also drops appdirs (an fs-only transitive) from the binary license manifest; six stays because python-dateutil still needs it. Closes apache#6917 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4UQSRY1RAXr7gJVFLLT7h
|
Can you check if it's same as #6928? |
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6946 +/- ##
=========================================
Coverage 78.96% 78.96%
Complexity 3798 3798
=========================================
Files 1161 1161
Lines 46084 46085 +1
Branches 5110 5110
=========================================
+ Hits 36388 36389 +1
Misses 8076 8076
Partials 1620 1620
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ No material benchmark regressions detected🟢 2 better · 🔴 0 worse · ⚪ 13 noise (<±5%) · 0 without baseline
Baseline detailsLatest main
Raw CSVconfig_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,368.59,200,128000,543,0.331,17682.42,25759.69,25759.69
1,100,10,64,20,1670.49,2000,1280000,1197,0.731,84144.16,94150.73,94150.73
2,1000,10,64,20,14090.30,20000,12800000,1419,0.866,708614.04,754597.96,754597.96 |
|
@aicam please mark this as a draft as we are discussing on the solution. Thanks |
What changes were proposed in this PR?
fs(PyFilesystem2) is unmaintained — 2.4.16 (May 2022) is its final release, and it calls__import__("pkg_resources").declare_namespace(__name__)at import time. setuptools 82.0.0 removedpkg_resources, soimport fshard-crashes there, which is the only reasonamber/requirements.txthas carried asetuptoolspin since #4199. This PR removes the dependency and the pin.ExecutorManagerwas the only consumer, andfs'sTempFSis internallytempfile.mkdtemp+shutil.rmtree, so the swap is behavior-preserving.amber/src/main/python/core/architecture/managers/executor_manager.pyfs→tmp_dir(aPathfromtempfile.mkdtemp);self.fs.open(...)→ builtinopen;self.fs.close()→shutil.rmtree(..., ignore_errors=True). Lazy creation,sys.pathappend/remove, the never-materialized early-return guard, and the tolerated leak-on-force-kill semantics are all preserved.amber/src/test/python/core/architecture/managers/test_executor_manager.pymanager.fs.getsyspath("/")reads becomemanager.tmp_dir; both close tests now also assert the directory is gone from disk, and a third test covers the early return when no tmp directory was ever created.amber/requirements.txtfs==2.4.16and thesetuptools==80.10.2pin plus its comment.amber/LICENSE-binary-pythonfs==2.4.16andappdirs==1.4.4(an fs-only transitive).sixstays —python-dateutilstill needs it.Implementation notes:
tempfile.mkdtempis deliberate, nottempfile.TemporaryDirectory: the latter attaches a finalizer that would silently remove the directory on GC or interpreter exit, changing the leak-on-force-kill behavior the existing TODO documents.shutil.rmtree(..., ignore_errors=True)matchesTempFS's defaultignore_clean_errors=True.texera-udf-prefix so a leaked one is identifiable in/var/tmp.setuptoolsis not restored torequirements.txt: nothing in the closure needs it at runtime any more. It still arrives transitively at 81.0.0 through torch'ssetuptools<82inoperator-requirements.txt, which is harmless and invisible to the license check (pip-licensesexcludes system packages).One extra fix, in the same code path:
load_executor_definitionnow callsimportlib.invalidate_caches()before importing the file it just wrote. The import system caches a directory listing persys.pathentry and only refreshes it when the directory mtime changes, so a second UDF written into the same tmp directory within one mtime tick can be invisible to the finder and raiseModuleNotFoundError: No module named 'udf-vN'. This is not introduced here — checking outmain'sexecutor_manager.pyandtest_executor_manager.pywithfsinstalled reproduces it locally (4TestUpdateExecutorfailures in the same environment). It is fixed in this PR because the write-then-import path is exactly what is being rewritten, and a reviewer running the suite locally would otherwise attribute the flake to this change.Any related issues, documentation, discussions?
Closes #6917. Supersedes #6911 (the setuptools v83 bump, closed — it could not merge while
fswas present) and #6412 (setuptools<82, the interim bound). Also makes #6880 unnecessary: withfsgone there is nopkg_resourcesdeprecation warning left to silence. Original pin: #4199, re-affirmed by the #6110 pin audit.How was this PR tested?
Existing test cases plus the assertions added above, run locally on Python 3.12 in a clean venv built from
amber/requirements.txt+amber/dev-requirements.txtwith proto bindings generated viabin/python-proto-gen.sh(import fsin that venv raisesModuleNotFoundError, confirming nothing reaches it):pytest -m "not integration"→ 862 passed, 1 deselected, 1 xfailed. This exercisesExecutorManagerdirectly (test_executor_manager.py) and end to end through UDF loading (pytexera/udf/,handlers/control/test_initialize_executor_handler.py,test_update_executor_handler.py,runnables/test_main_loop.py).test_executor_manager.pyrun 5× consecutively → 23 passed each time, confirming theinvalidate_cachesfix is stable (the same file fails 3–4 cases per run without it, onmainas well).ruff checkandruff format --checkoversrc/main/python src/test/python→ clean.pip-licensesover the runtime closure +./bin/licensing/check_binary_deps.py --ignore-transitive-version python→OK: 109 Python packages match LICENSE-binary. Running that same package list againstmain'sLICENSE-binary-pythonreports exactlySTALE: appdirs==1.4.4, fs==2.4.16— i.e. the two removals here are precisely what is required, and nothing else.requirements.txt+operator-requirements.txt) betweenmainand this branch: exactlyfs==2.4.16andappdirs==1.4.4removed, nothing added,sixretained,setuptools80.10.2 → 81.0.0 via torch.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)