Publish the v0.4.0 benchmark baseline - #16
Merged
Conversation
📝 WalkthroughWalkthroughThe changes commit v0.4.0 benchmark results and update readiness documentation, revise benchmark ignore rules, and isolate scheduler end-to-end test storage using per-test temporary directories. ChangesVersioned benchmark baseline
Scheduler test isolation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/test_cli_schedule.py`:
- Around line 20-29: Convert _isolated_scheduler_store into a yield fixture and,
in a finally block, identify handlers added during the test, remove them from
the logger, and close them. Preserve the existing temporary scheduler path
monkeypatches while ensuring each test cleans up its file handlers and
associated resources.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e01c685-958b-4b50-ba2f-571f7c625617
📒 Files selected for processing (7)
.gitignorePRODUCTION_READINESS.mdREADME.mdbenchmarks/results/10m.jsonbenchmarks/results/1m.jsonbenchmarks/results/README.mdtests/e2e/test_cli_schedule.py
Comment on lines
+20
to
+29
| @pytest.fixture(autouse=True) | ||
| def _isolated_scheduler_store(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: | ||
| """Keep CLI schedule tests out of the user's persistent job store.""" | ||
| import loafer.scheduler as scheduler | ||
|
|
||
| loafer_dir = tmp_path / ".loafer" | ||
| monkeypatch.setattr(scheduler, "_LOAFER_DIR", loafer_dir) | ||
| monkeypatch.setattr(scheduler, "_DB_PATH", loafer_dir / "jobs.db") | ||
| monkeypatch.setattr(scheduler, "_LOG_PATH", loafer_dir / "scheduler.log") | ||
| monkeypatch.setattr(scheduler, "_RUN_STATE_PATH", loafer_dir / "run_state.json") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close temporary log handlers after each test.
configure_file_logging() retains handlers globally, but this fixture creates a unique _LOG_PATH for every test. Its path-based deduplication therefore adds another FileHandler on each test, leaving open descriptors and duplicate writes to removed temporary files. Convert the fixture to a yield fixture and remove/close handlers added during the test in finally.
Proposed cleanup
+from collections.abc import Iterator
+
`@pytest.fixture`(autouse=True)
-def _isolated_scheduler_store(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
+def _isolated_scheduler_store(
+ tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> Iterator[None]:
"""Keep CLI schedule tests out of the user's persistent job store."""
import loafer.scheduler as scheduler
+ handlers_before = list(scheduler.logger.handlers)
loafer_dir = tmp_path / ".loafer"
monkeypatch.setattr(scheduler, "_LOAFER_DIR", loafer_dir)
monkeypatch.setattr(scheduler, "_DB_PATH", loafer_dir / "jobs.db")
monkeypatch.setattr(scheduler, "_LOG_PATH", loafer_dir / "scheduler.log")
monkeypatch.setattr(scheduler, "_RUN_STATE_PATH", loafer_dir / "run_state.json")
+ try:
+ yield
+ finally:
+ for handler in list(scheduler.logger.handlers):
+ if handler not in handlers_before:
+ scheduler.logger.removeHandler(handler)
+ handler.close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.fixture(autouse=True) | |
| def _isolated_scheduler_store(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: | |
| """Keep CLI schedule tests out of the user's persistent job store.""" | |
| import loafer.scheduler as scheduler | |
| loafer_dir = tmp_path / ".loafer" | |
| monkeypatch.setattr(scheduler, "_LOAFER_DIR", loafer_dir) | |
| monkeypatch.setattr(scheduler, "_DB_PATH", loafer_dir / "jobs.db") | |
| monkeypatch.setattr(scheduler, "_LOG_PATH", loafer_dir / "scheduler.log") | |
| monkeypatch.setattr(scheduler, "_RUN_STATE_PATH", loafer_dir / "run_state.json") | |
| from collections.abc import Iterator | |
| `@pytest.fixture`(autouse=True) | |
| def _isolated_scheduler_store( | |
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | |
| ) -> Iterator[None]: | |
| """Keep CLI schedule tests out of the user's persistent job store.""" | |
| import loafer.scheduler as scheduler | |
| handlers_before = list(scheduler.logger.handlers) | |
| loafer_dir = tmp_path / ".loafer" | |
| monkeypatch.setattr(scheduler, "_LOAFER_DIR", loafer_dir) | |
| monkeypatch.setattr(scheduler, "_DB_PATH", loafer_dir / "jobs.db") | |
| monkeypatch.setattr(scheduler, "_LOG_PATH", loafer_dir / "scheduler.log") | |
| monkeypatch.setattr(scheduler, "_RUN_STATE_PATH", loafer_dir / "run_state.json") | |
| try: | |
| yield | |
| finally: | |
| for handler in list(scheduler.logger.handlers): | |
| if handler not in handlers_before: | |
| scheduler.logger.removeHandler(handler) | |
| handler.close() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/test_cli_schedule.py` around lines 20 - 29, Convert
_isolated_scheduler_store into a yield fixture and, in a finally block, identify
handlers added during the test, remove them from the logger, and close them.
Preserve the existing temporary scheduler path monkeypatches while ensuring each
test cleans up its file handlers and associated resources.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
v0.4.0measurements~/.loafer/jobs.dbWhy
Phase 0 implementation was complete, but its benchmark evidence was still described as working-tree-only. The release gate required rerunning the capped matrix from an immutable revision and committing the resulting artifacts. During final verification, the scheduler CLI tests also exposed an isolation defect by attempting to write to the user's real scheduler database.
Impact
The repository now contains an auditable public-alpha baseline:
Root cause
The earlier benchmark measurements were produced from a mutable working tree and
benchmarks/results/was ignored, so there was no versioned release artifact. Separately, the scheduler E2E tests invoked the real defaultPath.home() / ".loafer"store instead of overriding all scheduler paths for test isolation.Validation
uv run pytest -q— 673 passed, 50 skippeduv run pytest tests/unit/test_full_pipeline_benchmark.py -q— 4 passeduv run pytest tests/e2e/test_cli_schedule.py -q— 5 passeduv run ruff check tests/e2e/test_cli_schedule.py benchmarks/full_pipeline.py— cleanv0.4.0image smoke pipeline — passedv0.4.0wheel smoke pipeline — passedruff check .andruff format --check .— passedSummary by CodeRabbit
Documentation
Tests