Skip to content

LCORE-1249: benchmarks devel dependency#1088

Merged
tisnik merged 2 commits intolightspeed-core:mainfrom
tisnik:lcore-1249-benchmarks-devel-dependency
Feb 1, 2026
Merged

LCORE-1249: benchmarks devel dependency#1088
tisnik merged 2 commits intolightspeed-core:mainfrom
tisnik:lcore-1249-benchmarks-devel-dependency

Conversation

@tisnik
Copy link
Contributor

@tisnik tisnik commented Feb 1, 2026

Description

LCORE-1249: benchmarks devel dependency

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks

Tools used to create PR

  • Assisted-by: N/A
  • Generated by: N/A

Related Tickets & Documents

  • Related Issue #LCORE-1249

Summary by CodeRabbit

  • New Features

    • Added performance benchmarking support and a Make target to run benchmarks.
  • Documentation

    • Updated Make targets list to include: run-llama-stack, test-e2e-local, generate-documentation, konflux-requirements, and reorganized help information.
  • Chores

    • Added pytest-benchmark to development dependencies.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 1, 2026

Walkthrough

Adds a new Makefile target "benchmarks" to run pytest against tests/benchmarks, documents various make targets in README.md, and adds pytest-benchmark>=5.2.3 to dev dependencies in pyproject.toml.

Changes

Cohort / File(s) Summary
Build & Dev deps
Makefile, pyproject.toml
Added benchmarks Make target to run pytest -q tests/benchmarks (via uv); added pytest-benchmark>=5.2.3 to dev dependencies.
Documentation
README.md
Updated Make targets section: added entries for help, run-llama-stack, test-e2e-local, generate-documentation, and konflux-requirements (consolidated target listings).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Suggested reviewers

  • radofuchs
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main change: adding benchmarks as a development dependency with the ticket reference LCORE-1249.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@README.md`:
- Line 772: The README contains a duplicate "help" Makefile/documentation target
entry; remove the redundant line so "help                              Show this
help screen" only appears once. Locate the duplicate entries for the "help"
target and delete one of them (keep the intended canonical occurrence), ensuring
any surrounding ordering or indexing remains consistent and no other lines
referencing the removed duplicate are left orphaned.
- Around line 742-744: The README duplicate entry for the "run" help target
should be removed: locate the duplicated "run                               Run
the service locally" lines in the help list (the repeated "run" target) and
delete the redundant occurrence so each help target is listed only once; verify
the help list still reads correctly and that no other targets were accidentally
overwritten.

README.md Outdated
Comment on lines +742 to +744
run Run the service locally
help Show this help screen
run Run the service locally
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Duplicate run target in documentation.

The run target appears twice in this list (lines 742 and 744). This appears to be a copy-paste error when updating the documentation.

Proposed fix
 run                               Run the service locally
 help                              Show this help screen
-run                               Run the service locally
 run-llama-stack                   Start Llama Stack with enriched config (for local service mode)
📝 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
run Run the service locally
help Show this help screen
run Run the service locally
run Run the service locally
help Show this help screen
🤖 Prompt for AI Agents
In `@README.md` around lines 742 - 744, The README duplicate entry for the "run"
help target should be removed: locate the duplicated "run                       
Run the service locally" lines in the help list (the repeated "run" target) and
delete the redundant occurrence so each help target is listed only once; verify
the help list still reads correctly and that no other targets were accidentally
overwritten.

@tisnik tisnik force-pushed the lcore-1249-benchmarks-devel-dependency branch from 5a06876 to 7ca4ba6 Compare February 1, 2026 11:56
@tisnik tisnik merged commit ecc7951 into lightspeed-core:main Feb 1, 2026
21 of 22 checks passed
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.

1 participant