Conversation
|
Damn, didn't thought copilot review would start automatically on another repo beside mines |
There was a problem hiding this comment.
Pull request overview
This PR adds an incremental output mode to the output contrib plugin to avoid wiping and rewriting entire pack directories on every build, addressing performance issues for large projects (issue #512).
Changes:
- Add
incremental: booltoOutputOptions(defaultFalse). - Implement
incremental_save()to delete removed files, prune empty directories, and skip writing unchanged files. - Update
output()to use incremental saving for non-zipped packs when enabled.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
should meta.output.incremental option be true by default? |
Honestly, yes imo. it's an improvement. |
I was just on a call with airdox, and he says it's not worth it on linux but windows would be good. |
|
AirDox just tested from my branch on linux:
|
rx-dev
left a comment
rx-dev
left a comment
There was a problem hiding this comment.
Some things were nitpicks, i realize the original code here used os than I realized though I really do think the pathlib apis are a bit cleaner and has better patterns which would cleanup more of the use-cases here.
There's a lot of functions written out here. Some is good organization, but some feels like over-doing it. Though, each one is described well so perhaps it would be good for future maintenance.
| if incremental is None: | ||
| meta_opts = ctx.meta.get("output") | ||
| if isinstance(meta_opts, dict): | ||
| incremental = bool(meta_opts.get("incremental", False)) |
There was a problem hiding this comment.
hmmm, i don't think u should be reading ctx.meta here, the way that @configurable works in the first place should auto-fill the opts with stuff from meta.
There was a problem hiding this comment.
@configurable does auto-fill from meta, but only when the plugin is called with no kwargs.
The toolchain registers this one as output(directory=ctx.output_directory) at project.py (line 315), and ctx.validate takes options=kwargs or None, so the meta branch at context.py (line 274) is skipped for every normal build.
It's only droppable if we make ctx.validate merge meta under the explicit kwargs, but since this changes the priority for each configurable plugin, it would be better to create a separate pull request.
|
Applied most of the review comments. Bench for the pathlib vs os: git clone https://github.com/Paralya/Switch
py bench_scan.py "Switch"Log of my execution PS D:\advanced_desktop\beet\bench> py .\bench_scan.py "D:\advanced_desktop\Switch"
D:\advanced_desktop\Switch (9969 files, 15 repeats)
scan_directory
os.scandir min 93.02 ms median 95.53 ms
os.walk + os.stat min 765.60 ms median 815.27 ms
Path.walk min 973.40 ms median 998.25 ms
Path.rglob min 1789.88 ms median 1882.13 ms
prune_empty_directories
os.walk + os.rmdir min 230.64 ms median 260.20 ms
Path.walk + Path.rmdir min 249.36 ms median 273.12 ms |
Fixes #512
A new incremental option (default to false) can be given to the output plugin.
either OutputOptions or in beet.yml:
When enabled:
File watchers will now be less in pain each build.
On my tests, I get faster build time, idk if you want to make it True by default so I put at False.
Bonus: Optimized the list_files() function that was building a Path to compute relative time for each file:
8f4d701