Skip to content

fix: keep async idle benchmark artifacts in scratch#683

Merged
eric-tramel merged 1 commit into
scheduling-yolofrom
andreatgretel/fix/async-scheduling-review-cleanup
May 19, 2026
Merged

fix: keep async idle benchmark artifacts in scratch#683
eric-tramel merged 1 commit into
scheduling-yolofrom
andreatgretel/fix/async-scheduling-review-cleanup

Conversation

@andreatgretel
Copy link
Copy Markdown
Contributor

Summary

  • write async idle benchmark defaults under ignored .scratch/ paths
  • keep generated report links relative to the report location
  • add regression coverage for default paths and artifact links

Why

The idle benchmark scripts previously wrote generated artifacts and reports into tracked repository paths by default. Running the documented smoke flow could leave an untracked worktree.

Validation

  • .venv/bin/pytest packages/data-designer-engine/tests/engine/test_async_scheduling_benchmark.py
  • .venv/bin/ruff check --fix .
  • .venv/bin/ruff format .

@eric-tramel eric-tramel marked this pull request as ready for review May 19, 2026 21:20
@eric-tramel eric-tramel requested a review from a team as a code owner May 19, 2026 21:20
@eric-tramel eric-tramel merged commit 0ee8f6d into scheduling-yolo May 19, 2026
4 checks passed
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 19, 2026

Greptile Summary

This PR moves the default output directories for the async idle benchmark scripts into .scratch/ (a gitignored path) so that running the documented smoke flow no longer leaves untracked files in the worktree. It also fixes _relative_href to compute proper relative links using os.path.relpath instead of always prepending ../.

  • Default artifact and report paths in both generate_async_scheduling_idle_report.py and run_async_scheduling_idle_regression.py are updated to .scratch/async-scheduling-idle-*/.
  • _relative_href now correctly handles the case where the report and its linked artifacts share a common parent directory (previously it would produce broken ../artifacts/… links when both were already under the same folder).
  • Two new tests are added to pin the default paths to .scratch/ and to verify the relative-href logic for same-directory and parent-directory cases.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to default path constants and a relative-URL helper, with no impact on data-processing logic.

Both the path change and the _relative_href fix are straightforward and well-covered by the new tests. The os.path.relpath replacement correctly handles same-directory and cross-directory cases, and the ValueError guard for cross-drive paths on Windows is preserved.

No files require special attention.

Important Files Changed

Filename Overview
scripts/benchmarks/generate_async_scheduling_idle_report.py Redirects default output paths to .scratch/ and fixes _relative_href to use os.path.relpath instead of hardcoded ../ prefix.
scripts/benchmarks/run_async_scheduling_idle_regression.py Mirrors the default path change from generate_async_scheduling_idle_report.py, redirecting output to .scratch/async-scheduling-idle-regression/.
packages/data-designer-engine/tests/engine/test_async_scheduling_benchmark.py Adds two regression tests: one asserting default paths are under .scratch/, and one asserting _relative_href computes correct relative paths in sibling and parent-directory cases.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Script invoked] --> B{artifact_dir / report_path\nprovided via CLI?}
    B -- Yes --> C[Use supplied paths]
    B -- No --> D[Use DEFAULT_ARTIFACT_DIR\nDEFAULT_REPORT_PATH]
    D --> E[.scratch/async-scheduling-idle-*/artifacts\n.scratch/async-scheduling-idle-*.html]
    C --> F[_relative_href\nreport_path x target_path]
    E --> F
    F --> G{target_path\nabsolute?}
    G -- Yes --> H[target_path.as_uri]
    G -- No --> I[os.path.relpath\ntarget_path, start=report_path.parent]
    I --> J[Path.as_posix - relative href in HTML]
    H --> J
Loading

Reviews (1): Last reviewed commit: "fix: keep async idle benchmark artifacts..." | Re-trigger Greptile

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