Skip to content

Add files_size to con-duct ls output#406

Draft
Copilot wants to merge 5 commits intomainfrom
copilot/add-files-size-to-ls-f
Draft

Add files_size to con-duct ls output#406
Copilot wants to merge 5 commits intomainfrom
copilot/add-files-size-to-ls-f

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 8, 2026 •
edited
Loading

Users accumulating large log directories had no quick way to correlate session file sizes with other execution metrics like duration and memory usage.

Changes

  • New files_size field: Computed at query time by globbing all files matching the session prefix and summing their sizes. Not stored in the info JSON — computed on the fly in load_duct_runs.
  • Humanized display: Uses the existing !S conversion ("{value!S}"), so it renders as e.g. 4.2 MB in summaries/pyout output and as raw bytes in json/json_pp/yaml.
  • Default field: Added to the default --fields list so it shows up without any extra flags.
  • COMPUTED_FIELDS list: Distinguishes fields computed at query time from those present in the info JSON schema, allowing the schema conformance test to exclude them.

Example

$ con-duct ls
Prefix: .duct/logs/2024.03.01T12.00.00_
  Command: python train.py
  Exit Code: 0
  Wall Clock Time: 3621.042 sec
  Peak Rss: 12.4 GB
  Files Size: 587.3 MB

Copilot AI linked an issue Apr 8, 2026 that may be closed by this pull request
Include files_size into ls -F #405
Open
@yarikoptic yarikoptic added the semver-minor Increment the minor version when merged label Apr 8, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.70%. Comparing base (2162da9) to head (19ecc38).

Files with missing lines Patch % Lines
src/con_duct/ls.py 76.92% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
- Coverage   91.87%   91.70%   -0.18%     
==========================================
  Files          15       15              
  Lines        1120     1133      +13     
  Branches      139      141       +2     
==========================================
+ Hits         1029     1039      +10     
- Misses         69       71       +2     
- Partials       22       23       +1

☔ View full report in Codecov by Sentry.
📢 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.

Copilot AI and others added 2 commits April 8, 2026 11:56
@yarikoptic
Add files_size field to ls -F output
57ddd55 
- Add `files_size` to `VALUE_TRANSFORMATION_MAP` with `{value!S}` (humanized size)
- Add `COMPUTED_FIELDS` list to distinguish computed fields from JSON schema fields
- Add `compute_files_size()` function that sums sizes of all session files via glob
- Compute `files_size` in `load_duct_runs` after loading each run
- Add `files_size` to default `--fields` in `_create_ls_parser`
- Update `test_schema.py` to exclude computed fields from schema equality check
- Add tests for `compute_files_size`, `load_duct_runs`, and `ls` output

Agent-Logs-Url: https://github.com/con/duct/sessions/829c6010-40e6-416a-99be-6f9361c06057

Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com>
@yarikoptic
Address code review feedback: improve docstring and test clarity
19ecc38 
Agent-Logs-Url: https://github.com/con/duct/sessions/829c6010-40e6-416a-99be-6f9361c06057

Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com>
Copilot AI changed the title [WIP] Add files_size to ls -F command Add files_size to con-duct ls output Apr 8, 2026
Copilot AI requested a review from yarikoptic April 8, 2026 12:02
yarikoptic
yarikoptic reviewed Apr 8, 2026
View reviewed changes
src/con_duct/ls.py Outdated
if path.is_file():
try:
total += path.stat().st_size
except OSError:
Copy link
Copy Markdown
Member

@yarikoptic yarikoptic Apr 8, 2026

Choose a reason for hiding this comment

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

warrants at least logging for debug

Copy link
Copy Markdown
Member

@yarikoptic yarikoptic Apr 8, 2026

Choose a reason for hiding this comment

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

@copilot add logging ... since unexpected, I think it could even be WARNING

Copy link
Copy Markdown
Contributor Author

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

Added lgr.debug("Could not get size of %s: %s", path_str, e) in the except OSError block in ca997ba.

Copy link
Copy Markdown
Contributor Author

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

Upgraded to lgr.warning(...) in c19eaf9.

yarikoptic
yarikoptic reviewed Apr 8, 2026
View reviewed changes
test/test_ls.py Outdated
result = self._run_ls(["file1_info.json"], "summaries", args)
assert "Files Size:" in result
# Should show humanized size (Bytes or kB etc.) not raw integer
assert any(unit in result for unit in ["Byte", "kB", "MB", "GB"])
Copy link
Copy Markdown
Member

@yarikoptic yarikoptic Apr 8, 2026

Choose a reason for hiding this comment

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

I think some of those tests warrant just addition of testing within existing tests instead of breeding their own -- I bet we do not have a test per each column. @copilot please review and adjust to retain coverage and testing but reduce code added in the tests

Copy link
Copy Markdown
Contributor Author

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

Consolidated in ca997ba:

  • Merged the empty-prefix case into test_compute_files_size_sums_all_files as a second assertion
  • Removed test_load_duct_runs_includes_files_size — added assert "files_size" in result[0] to test_load_duct_runs_sanity instead
  • Removed test_ls_files_size_in_output and test_ls_files_size_humanized_in_summaries — added files_size to the _run_ls helper's default fields, then added the assertions to test_ls_sanity (humanized check) and test_ls_json_output (field presence check)

@yarikoptic
Address review: debug log on OSError, consolidate files_size tests in…
ca997ba 
…to existing tests

Agent-Logs-Url: https://github.com/con/duct/sessions/74a1b4cf-2d72-4835-af90-44d1e11414f3

Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com>
Copilot AI requested a review from yarikoptic April 8, 2026 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yarikoptic yarikoptic Awaiting requested review from yarikoptic

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

@yarikoptic yarikoptic

Copilot code review Copilot

Labels

semver-minor Increment the minor version when merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Include files_size into ls -F

2 participants

@yarikoptic