Skip to content

refactor(pyamber): drop PyFilesystem2 for stdlib tempfile - #6928

Closed
aglinxinyuan wants to merge 2 commits into
apache:mainfrom
aglinxinyuan:refactor/remove-pyfilesystem2
Closed

refactor(pyamber): drop PyFilesystem2 for stdlib tempfile#6928
aglinxinyuan wants to merge 2 commits into
apache:mainfrom
aglinxinyuan:refactor/remove-pyfilesystem2

Conversation

@aglinxinyuan

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

fs (PyFilesystem2) is effectively EOL — 2.4.16 (2022) is the last release ever published, upstream still imports pkg_resources at import time, and setuptools 82 removed pkg_resources entirely, which is the sole reason amber pins setuptools==80.10.2 (#4199, #6412). ExecutorManager was the only consumer, and fs's TempFS is internally just tempfile.mkdtemp + shutil.rmtree, so the swap to stdlib is behavior-preserving.

Before:  ExecutorManager -> fs.open_fs("temp://") -> pkg_resources -> setuptools held < 82
After:   ExecutorManager -> tempfile.mkdtemp + shutil.rmtree -> fs gone -> setuptools unpinned
File Change
executor_manager.py fs.open_fs("temp://")tempfile.mkdtemp(prefix="texera-udf-") + shutil.rmtree; property renamed fstmp_dir; lazy creation, sys.path handling, the never-materialized close guard, and the tolerated leak-on-force-kill semantics all preserved
test_executor_manager.py the two direct manager.fs accesses use manager.tmp_dir; new non-ASCII UDF round-trip regression test (see below)
requirements.txt drop fs==2.4.16 and the setuptools==80.10.2 pin — fs was the only distribution in the closure requiring setuptools (pytz's pkg_resources use is a guarded fallback)
LICENSE-binary-python drop fs==2.4.16 and appdirs==1.4.4 (an fs-only transitive); six stays (python-dateutil needs it)
texera_run_python_worker.py, pyproject.toml revert the #6880 warning filters — the warning's source is gone, so vs main these files are net-untouched (pyproject keeps only an EOF-newline fix)

One subtlety worth reviewer attention: fs's open() forced UTF-8 for text writes, while pathlib's open("w") uses the locale encoding — on a cp1252 Windows host, non-ASCII UDF code would crash at write time or fail at import (importlib decodes source as UTF-8 per PEP 3120). The swap therefore pins encoding="utf-8", newline="\n" explicitly, keeping byte-identical behavior with fs, and a regression test pins it.

Stacked on #6880 (contains its commit); whichever merges first works — if this one lands first, #6880 can simply be closed.

Any related issues, documentation, discussions?

Closes #6917.

How was this PR tested?

  • New test_non_ascii_udf_source_round_trip written first: it fails with UnicodeEncodeError under a cp1252 locale without the explicit encoding="utf-8" (reproduced on Windows), and passes with it — pinning encoding parity with fs.
  • The two close() branch tests were repointed at tmp_dir red-first (failing on the old attribute) before the source swap; all 23 test_executor_manager.py tests pass.
  • Full pyamber unit suite from amber/: 750 passed, identical to the pre-change baseline apart from the added test (the handful of Iceberg-catalog failures on this Windows machine are identical on a clean tree). The suite's pkg_resources warnings disappeared at the source (5 → 1 total warnings) with no filter in place.
  • Behavior parity was checked against fs's own implementation (fs/tempfs.py, fs/iotools.py): directory creation, close/cleanup semantics including double-close, and the cached-property guard translate 1:1.
  • ruff check and ruff format --check pass on all touched files.

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

Generated-by: Claude Code (Fable 5)

Every spawned Python UDF worker imports fs (PyFilesystem2), whose import
of pkg_resources emits a UserWarning on stderr. Filter that one message
before the core import chain, and mirror the filter for pytest output.
Follow-up item from apache#6796.
fs (PyFilesystem2) is EOL: 2.4.16 (2022) is the last release ever and
its import-time pkg_resources use is the sole reason setuptools was
pinned below 82 (apache#4199, apache#6412). ExecutorManager was the only consumer;
its temp:// filesystem becomes tempfile.mkdtemp + shutil.rmtree, with
the UDF source encoding pinned to UTF-8 exactly as fs did (new
non-ASCII round-trip regression test). Removes the fs and setuptools
pins, syncs LICENSE-binary-python (drops fs-only transitive appdirs),
and reverts the now-unneeded pkg_resources warning filters from apache#6880.

Closes apache#6917.
@github-actions github-actions Bot added dependencies Pull requests that update a dependency file pyamber refactor Refactor the code labels Jul 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @Ma77Ball, @Yicong-Huang
    You can notify them by mentioning @Ma77Ball, @Yicong-Huang in a comment.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.66%. Comparing base (429be11) to head (849a48c).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6928      +/-   ##
============================================
+ Coverage     77.31%   77.66%   +0.35%     
  Complexity     3524     3524              
============================================
  Files          1161     1161              
  Lines         45920    46043     +123     
  Branches       5099     5099              
============================================
+ Hits          35501    35760     +259     
+ Misses         8839     8703     -136     
  Partials       1580     1580              
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from d13da15
agent-service 76.76% <ø> (ø) Carriedforward from d13da15
amber 69.10% <ø> (ø) Carriedforward from d13da15
computing-unit-managing-service 20.49% <ø> (ø) Carriedforward from d13da15
config-service 66.66% <ø> (ø) Carriedforward from d13da15
file-service 67.21% <ø> (ø) Carriedforward from d13da15
frontend 82.58% <ø> (ø) Carriedforward from d13da15
notebook-migration-service 78.94% <ø> (ø) Carriedforward from d13da15
pyamber 95.38% <100.00%> (+3.23%) ⬆️
workflow-compiling-service 55.14% <ø> (ø) Carriedforward from d13da15

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aglinxinyuan
aglinxinyuan requested a review from mengw15 July 27, 2026 07:46
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 0 better · 🔴 2 worse · ⚪ 13 noise (<±5%) · 0 without baseline

Compared against main 49f9e2c benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 401 0.244 24,463/37,126/37,126 us 🔴 +11.4% / 🔴 +126.8%
bs=100 sw=10 sl=64 804 0.491 124,583/152,331/152,331 us ⚪ within ±5% / 🔴 +39.4%
bs=1000 sw=10 sl=64 911 0.556 1,093,537/1,177,616/1,177,616 us ⚪ within ±5% / 🔴 +12.1%
Baseline details

Latest main 49f9e2c from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 401 tuples/sec 412 tuples/sec 759.72 tuples/sec -2.7% -47.2%
bs=10 sw=10 sl=64 MB/s 0.244 MB/s 0.251 MB/s 0.464 MB/s -2.8% -47.4%
bs=10 sw=10 sl=64 p50 24,463 us 24,037 us 12,653 us +1.8% +93.3%
bs=10 sw=10 sl=64 p95 37,126 us 33,336 us 16,371 us +11.4% +126.8%
bs=10 sw=10 sl=64 p99 37,126 us 33,336 us 19,941 us +11.4% +86.2%
bs=100 sw=10 sl=64 throughput 804 tuples/sec 817 tuples/sec 966.78 tuples/sec -1.6% -16.8%
bs=100 sw=10 sl=64 MB/s 0.491 MB/s 0.499 MB/s 0.59 MB/s -1.6% -16.8%
bs=100 sw=10 sl=64 p50 124,583 us 121,341 us 103,654 us +2.7% +20.2%
bs=100 sw=10 sl=64 p95 152,331 us 150,049 us 109,308 us +1.5% +39.4%
bs=100 sw=10 sl=64 p99 152,331 us 150,049 us 116,369 us +1.5% +30.9%
bs=1000 sw=10 sl=64 throughput 911 tuples/sec 913 tuples/sec 997.95 tuples/sec -0.2% -8.7%
bs=1000 sw=10 sl=64 MB/s 0.556 MB/s 0.557 MB/s 0.609 MB/s -0.2% -8.7%
bs=1000 sw=10 sl=64 p50 1,093,537 us 1,100,192 us 1,007,348 us -0.6% +8.6%
bs=1000 sw=10 sl=64 p95 1,177,616 us 1,137,155 us 1,050,149 us +3.6% +12.1%
bs=1000 sw=10 sl=64 p99 1,177,616 us 1,137,155 us 1,079,497 us +3.6% +9.1%
Raw CSV
config_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,499.27,200,128000,401,0.244,24462.71,37125.55,37125.55
1,100,10,64,20,2487.95,2000,1280000,804,0.491,124582.85,152331.17,152331.17
2,1000,10,64,20,21951.00,20000,12800000,911,0.556,1093537.15,1177616.10,1177616.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file pyamber refactor Refactor the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate off unmaintained PyFilesystem2 (fs) and unpin setuptools

2 participants