ci: add import-time gate for rogue (<= 1.0s best-of-5 cold imports)#104
ci: add import-time gate for rogue (<= 1.0s best-of-5 cold imports)#104yuval-qf merged 6 commits intoqualifire-dev:mainfrom
Conversation
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit
WalkthroughAdded a new GitHub Actions workflow (.github/workflows/import-time.yml) that runs on push/PR/manual, sets PACKAGE_NAME and THRESHOLD_SECONDS, creates a venv, installs the repo and sdks/python, measures package import time across five attempts using /usr/bin/time, and fails if the best time exceeds the threshold. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as CI Runner
participant Shell as Shell (/usr/bin/time)
participant Py as Python (venv)
participant PKG as Target Package
participant SDK as sdks/python
GH->>Runner: Trigger (push / PR / manual)
Runner->>Runner: Checkout repo\nread .python-version
Runner->>Runner: Setup Python & create venv
Runner->>Runner: pip install -e .
Runner->>Runner: pip uninstall rogue-ai-sdk (if present)
Runner->>SDK: pip install sdks/python
Runner->>Runner: ensure `time` and `bc` available
Runner->>Shell: Run 5 attempts: /usr/bin/time python -c "import PACKAGE_NAME"
Shell->>Py: spawn import attempts
Py->>PKG: import PACKAGE_NAME (repeated 5x)
PKG-->>Py: import completes
Shell-->>Runner: emit per-run elapsed times
Runner->>Runner: pick best_time (min)
alt best_time <= THRESHOLD_SECONDS
Runner-->>GH: exit 0 (success)
else best_time > THRESHOLD_SECONDS
Runner-->>GH: print failure to stderr\nexit 1 (failure)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @drorIvry @yuval-qf — please approve CI workflow runs for this PR. This adds .github/workflows/import-time.yml to enforce a 1.0s max best-of-5 cold import time for Happy to switch the install to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/import-time.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/import-time.yml (1)
47-52: Best-of-5 collection and threshold check look good.The logic correctly collects the five import times, reports the minimum, and fails the job if the threshold is exceeded. Output formatting is clear and helpful for debugging.
yuval-qf
left a comment
yuval-qf
left a comment
There was a problem hiding this comment.
Thanks for this PR!
Left a few comments
.github/workflows/import-time.yml
Outdated
| - name: Install package | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -e . |
There was a problem hiding this comment.
Please take a look on how we install rogue in other workflows.
Your method will not include the local sdk in /sdks/python/rogue_sdk which might result in some errors
.github/workflows/import-time.yml
Outdated
| run: | | ||
| python - << 'PY' | ||
| import importlib, os, sys | ||
| import subprocess, time | ||
|
|
||
| package = os.getenv("PACKAGE_NAME", "rogue") | ||
| threshold = float(os.getenv("THRESHOLD_SECONDS", "1.0")) | ||
|
|
||
| def cold_import_once(pkg: str) -> float: | ||
| code = ( | ||
| "import importlib, time; " | ||
| f"t=time.perf_counter(); importlib.import_module('{pkg}'); " | ||
| "print(time.perf_counter()-t)" | ||
| ) | ||
| try: | ||
| out = subprocess.check_output([sys.executable, "-c", code], text=True).strip() | ||
| return float(out) | ||
| except subprocess.CalledProcessError as e: | ||
| err = getattr(e, "stderr", None) | ||
| if err is None: | ||
| err = "(no stderr captured)" | ||
| print(f"FAIL: Import subprocess failed with exit code {e.returncode}: {err}", file=sys.stderr) | ||
| sys.exit(1) | ||
| except ValueError: | ||
| print(f"FAIL: Could not parse import time from output: {out!r}", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| times = [cold_import_once(package) for _ in range(5)] | ||
| best = min(times) | ||
| print(f"Best import time for '{package}': {best:.4f} s (threshold {threshold:.2f} s)") | ||
| if best > threshold: | ||
| print("FAIL: Import time exceeded threshold.", file=sys.stderr) | ||
| sys.exit(1) | ||
| PY |
There was a problem hiding this comment.
I think there is a much cleaner, and more readable way to do this:
something like:
apt-get update && apt-get install -y time # this can be done in a different step
elapsed=$(time -f "%e" python -c "import rogue")
echo "Rogue was imported in $elapsed seconds"
# Need to use `bc` to compare floating point numbers in bash
if echo "$elapsed < $THRESHOLD" | bc -l | grep -q 1; then
echo "Pass"
else
echo "Fail"
exit 1
fi
|
Hi @yuval-qf, thanks for the review — I’ve addressed your comments.
Changes in /.github/workflows/import-time.yml (latest commit: eed5cbf). If you’d like this to trigger only on PRs (not pushes), I can scope on: to pull_request. I can also sync the branch with main if preferred. |
yuval-qf
left a comment
yuval-qf
left a comment
There was a problem hiding this comment.
Thanks for this contribution!
Happy to approve!
2 participants
Summary
Add a CI job that measures the cold import time of the
roguepackage and fails the build if the best-of-5 import time exceeds 1.0s.This is implemented as a new workflow: .github/workflows/import-time.yml.
Motivation
We’ve invested in import-time optimizations (e.g.,
if TYPE_CHECKING, deferring heavy imports) and want to prevent regressions. Consistently enforcing an upper bound on import time helps preserve fast startup, especially for CLI/TUI entry points and server processes.What’s Changed
pushandpull_request.pip install -e ..THRESHOLD_SECONDS(default 1.0s).Implementation Details
python -c "import importlib, time; t=time.perf_counter(); importlib.import_module('rogue'); print(time.perf_counter()-t)"in a fresh subprocess 5 times.THRESHOLD_SECONDSenv var (defaults to1.0).Trade-offs
pip install -e .in CI for simplicity and isolation of the PR’s code. If project-wide CI is standardized onuv, we can switch touv runequivalents in a follow-up without changing the measurement approach.How to Test Locally