Skip to content

feat(test-harness): add --keep-outputs flag and refactor Runner to Options struct#2942

Merged
markphelps merged 4 commits intomainfrom
test-harness-keep-outputs
Apr 15, 2026
Merged

feat(test-harness): add --keep-outputs flag and refactor Runner to Options struct#2942
markphelps merged 4 commits intomainfrom
test-harness-keep-outputs

Conversation

@markphelps
Copy link
Copy Markdown
Contributor

@markphelps markphelps commented Apr 14, 2026

Summary

  • --keep-outputs flag: Preserves the work directory (prediction output files like images) after a run, printing the path so they can be inspected instead of being automatically deleted.
  • Keep Docker images by default: Replaces --keep-images with --clean-images. Images are now kept by default to avoid wasteful rebuilds during iterative local development.
  • Runner Options refactor: Replaces the 6-argument runner.New() constructor with a runner.Options struct for clarity and extensibility.
  • MIME type detection fix: validateFileExists was passing the full file path to mime.TypeByExtension instead of just the extension (e.g. .png), causing MIME checks to always return empty and fail.
  • Test convention cleanup: Converts patcher and validator tests from raw t.Fatal/t.Errorf to testify assert/require, matching the project convention. Removes hand-rolled contains/mapsEqual helpers.

Usage

# Keep outputs for inspection
go run . run --no-gpu --keep-outputs --model blur
# => Outputs preserved in: ~/.cache/cog-harness/run-XXXXXXX

# Explicitly clean up Docker images after run
go run . run --no-gpu --clean-images

…e Options struct

Adds a --keep-outputs flag that preserves the work directory (and its
prediction output files like images) after a run, printing the path so
they can be inspected. Also refactors runner.New() from 6 positional
arguments to a runner.Options struct for clarity and extensibility.
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

Found a minor documentation inconsistency:

Comment thread tools/test-harness/README.md Outdated
--cog-ref REF Git ref to build cog from source
--sdk-wheel PATH Local SDK wheel path
--keep-images Keep Docker images after run
--keep-outputs Preserve prediction outputs (images, files) in the work directory
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The help text here says "Preserve prediction outputs (images, files) in the work directory" but doesn't mention that the path will be printed. Consider updating to:

Suggested change
--keep-outputs Preserve prediction outputs (images, files) in the work directory
--keep-outputs Preserve prediction outputs and print the work directory path

This better matches the actual behavior in runner.go:134 where it prints "Outputs preserved in: %s".

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

Posted a review on PR #2942. The only issue I found is a minor documentation inconsistency - the README mentions that --keep-outputs preserves files but doesn't mention that the work directory path is also printed, which is a key part of the feature for users who want to inspect those outputs.

github run

Fix validateFileExists passing the full file path to mime.TypeByExtension
instead of just the extension, causing MIME checks to always fail.

Convert patcher and validator tests from raw t.Fatal/t.Errorf to
testify assert/require, matching the project convention.
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

Found a minor documentation inconsistency:

Comment thread tools/test-harness/README.md Outdated
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

Found a minor documentation inconsistency: the README help text for --keep-outputs doesn't mention that the path will be printed (the actual behavior prints "Outputs preserved in: "). Posted a suggestion comment on line 88 of tools/test-harness/README.md.

github run

…ages

Docker image builds are expensive. Keeping them by default avoids
wasteful rebuilds during iterative local development. Replace
--keep-images with --clean-images for when explicit cleanup is wanted.
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

LGTM

github run

@markphelps markphelps marked this pull request as ready for review April 14, 2026 22:25
@markphelps markphelps requested a review from a team as a code owner April 14, 2026 22:25
@markphelps markphelps requested a review from michaeldwan April 14, 2026 22:25
Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

LGTM

github run

@markphelps markphelps merged commit 04aa420 into main Apr 15, 2026
32 checks passed
@markphelps markphelps deleted the test-harness-keep-outputs branch April 15, 2026 12:30
michaeldwan added a commit that referenced this pull request Apr 15, 2026
…conditionally

errcheck violations (unchecked error returns on Cleanup, RemoveAll,
Close, Cmd.Run) and gocritic octal literal style (0644 -> 0o644).
These were introduced in #2942 and went undetected because CI skipped
Go lint when only Go files in tools/ changed.
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.

2 participants