Skip to content

perf(pull): apply CANFAR permissions without subprocesses - #157

Merged
djgormley merged 6 commits into
mainfrom
perf-optimize-chown-chmod-17468975491131157657
Sep 29, 2026
Merged

djgormley merged 6 commits into
mainfrom
perf-optimize-chown-chmod-17468975491131157657

Conversation

@tjzegmott

@tjzegmott tjzegmott commented May 19, 2026 •

Copy link
Copy Markdown
Contributor

CANFAR downloads currently start separate recursive chgrp and chmod processes for every destination folder. Replace those subprocesses with native traversal, resolve chime-frb-rw once per transfer, and preserve existing mode bits while adding group write permission. Permission failures remain best effort, and symlinks are not followed.

The branch now includes current main; its final diff preserves the newer JSON output, transfer handling, and dependency updates.

Validation:

  • 273 non-CADC tests pass on Python 3.10 and 3.14; 9 credential-dependent tests are excluded.
  • 10 new permission tests cover nested files, symlinks and replacement races, missing groups, unreadable owned files, and per-file failures.
  • All pre-commit hooks pass.
  • A local temporary-directory benchmark measured 89.3 ms to 3.10 ms across 40 small directories. A single directory with 2,000 files measured 15.6 ms to 17.3 ms, so the benefit is reduced process-launch overhead rather than a universal speedup. CANFAR filesystem performance has not been measured.

Replaced `os.system` subshell spawns with native Python `os.walk`, `shutil.chown`, and `os.chmod` inside `get_files()` to vastly improve performance and remove a shell injection vector.

Co-authored-by: tjzegmott <20817254+tjzegmott@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 19, 2026 18:41
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Replaces shell-based chgrp/chmod invocations in get_files() with native Python equivalents (os.walk + shutil.chown + os.chmod) on the canfar site to remove per-folder subprocess overhead.

Changes:

  • Added import stat.
  • Replaced two os.system(...) calls per folder with an os.walk loop that applies group ownership (chime-frb-rw) and g+w bit using native APIs.
  • Preserves the prior fire-and-forget semantics by swallowing OSError per file/directory.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Replaced `os.system` subshell spawns with native Python `os.walk`, `shutil.chown`, and `os.chmod` inside `get_files()` to vastly improve performance and remove a shell injection vector. Also extracted to helper to resolve flake8 complexity.

Co-authored-by: tjzegmott <20817254+tjzegmott@users.noreply.github.com>
@coveralls

coveralls commented May 19, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 64.656% (-2.0%) from 66.667% — perf-optimize-chown-chmod-17468975491131157657 into main

@codecov-commenter

codecov-commenter commented May 19, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.93617% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.70%. Comparing base (f52a917) to head (9108c95).
⚠️ Report is 68 commits behind head on main.

Files with missing lines Patch % Lines
dtcli/src/functions.py 95.23% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #157       +/-   ##
===========================================
+ Coverage   67.08%   78.70%   +11.62%     
===========================================
  Files          16       31       +15     
  Lines        1686     4128     +2442     
===========================================
+ Hits         1131     3249     +2118     
- Misses        555      879      +324     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

google-labs-jules Bot and others added 3 commits May 20, 2026 21:01
Replaced `subprocess.run` subshell spawns with native Python `os.walk`, `shutil.chown`, and `os.chmod` inside `get_files()` to vastly improve performance. Also extracted to helper to resolve flake8 complexity.

Co-authored-by: tjzegmott <20817254+tjzegmott@users.noreply.github.com>
Co-authored-by: tjzegmott <20817254+tjzegmott@users.noreply.github.com>
@tjzegmott tjzegmott closed this Jul 21, 2026
@djgormley
djgormley deleted the perf-optimize-chown-chmod-17468975491131157657 branch August 25, 2026 17:59
@djgormley
djgormley restored the perf-optimize-chown-chmod-17468975491131157657 branch September 29, 2026 01:39
@djgormley djgormley reopened this Sep 29, 2026
@djgormley djgormley changed the title ⚡ Performance Improvement: Remove subshell bottlenecks for chmod and chgrp perf(pull): apply CANFAR permissions without subprocesses Sep 29, 2026
@djgormley
djgormley merged commit b8c2230 into main Sep 29, 2026
9 checks passed
@djgormley
djgormley deleted the perf-optimize-chown-chmod-17468975491131157657 branch September 29, 2026 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants