-
Notifications
You must be signed in to change notification settings - Fork 69
remove dependency of API agents: inline::meta-reference #963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Haoyu Sun <hasun@redhat.com>
WalkthroughThis pull request updates the project's dependency configuration by commenting out five packages in pyproject.toml's llslibdev group, introducing a new expanded set of runtime and tooling dependencies, and removing numerous pinned transitive dependencies and their associated hashes from platform-specific requirements files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pyproject.toml (1)
132-137: Clarify the comment: these packages are not used by inline::meta-reference agents, not unused in the projectThe commented-out dependencies at lines 132–137 are safe to remove from
llslibdev:matplotlib,pillow,pandas, andscikit-learnhave no direct imports in the codebase, andpsycopg2-binaryis correctly retained as a top-level dependency since it's actively used insrc/utils/quota.py,src/quota/quota_limiter.py,src/quota/connect_pg.py,src/cache/postgres_cache.py, andsrc/quota/token_usage_history.py.However, the comment "not used in the project" is misleading. Since
psycopg2-binaryis directly imported and required by llama-stack providers, clarify the reason for commenting them out:- # commented out because they are not used in the project + # commented out because inline::meta-reference agents are no longer usedThis avoids confusion about whether the packages are still required elsewhere (which they are, transitively or directly).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomlrequirements.aarch64.txtrequirements.x86_64.txt
🧰 Additional context used
📓 Path-based instructions (1)
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: Configure pylint withsource-roots = "src"
Excludesrc/auth/k8s.pyfrom pyright type checking
Files:
pyproject.toml
🧠 Learnings (3)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Always check `pyproject.toml` for existing dependencies and versions before adding new ones
Applied to files:
pyproject.toml
📚 Learning: 2025-08-18T10:58:14.951Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:47-47
Timestamp: 2025-08-18T10:58:14.951Z
Learning: psycopg2-binary is required by some llama-stack providers in the lightspeed-stack project, so it cannot be replaced with psycopg v3 or moved to optional dependencies without breaking llama-stack functionality.
Applied to files:
pyproject.tomlrequirements.x86_64.txtrequirements.aarch64.txt
📚 Learning: 2025-08-18T10:57:39.266Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:59-59
Timestamp: 2025-08-18T10:57:39.266Z
Learning: In the lightspeed-stack project, transitive dependencies like faiss-cpu are intentionally pinned as top-level dependencies to maintain better control over the dependency graph and avoid version conflicts when bundling ML/LLM tooling packages.
Applied to files:
requirements.x86_64.txtrequirements.aarch64.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / vertexai
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: library mode / vertexai
🔇 Additional comments (4)
requirements.x86_64.txt (1)
1902-1902: uv “via” annotations correctly reflect the updated dependency graphThe new
# via ...comments forpandas,psycopg2-binary, andscikit-learnline up with the currentpyproject.tomlsetup (direct vs transitive sources), so the regenerate-from-uv flow looks consistent and there’s nothing to fix in this file for these changes. Please just ensure this file was produced byuv pip compileafter the pyproject changes, not hand-edited.Also applies to: 2208-2208, 2921-2921
requirements.aarch64.txt (3)
1898-1904: Pandas still present transitively (now attributed tollama-stack)This change just updates the “via” metadata;
pandasremains in the lock as a transitive dep (now correctly attributed tollama-stack). No functional impact, but be aware this environment still carriespandaseven though it’s no longer top‑level inllslibdev.If your intention was to fully drop
pandasfrom this profile rather than just frompyproject.toml, please confirm that no remaining deps (e.g.,llama-stack) require it, or consider regenerating constraints without it.
2204-2210:psycopg2-binaryattribution matches project constraintsThe updated “via” annotation for
psycopg2-binarynow points atlightspeed-stack (pyproject.toml), which aligns with the requirement to keep this package for some llama-stack providers (and not move it to optional deps). No issues here.Given this is a critical DB driver, please double‑check that
pyproject.tomlstill keepspsycopg2-binaryin a non‑optional dependency group used by all relevant providers. Based on learnings, this is important for llama-stack compatibility.
2916-2922:scikit-learnremains as a transitive dep viasentence-transformersAlthough
scikit-learnwas removed from thellslibdevgroup inpyproject.toml, this lock file still pins it becausesentence-transformersdepends on it (as reflected by the new “via” comment). This is consistent with pinning heavy transitives for control over the graph, but it means the runtime still includesscikit-learn.If the goal of the PR is to slim the API agents’ dependency surface, consider whether keeping
sentence-transformers(and thusscikit-learn) in this env is desired, or if those usages can be isolated to another group. Based on learnings, transitive pins are intentional, but worth reconfirming here.
tisnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| # "pillow>=11.1.0", | ||
| # "pandas>=2.2.3", | ||
| # "scikit-learn>=1.5.2", | ||
| # "psycopg2-binary>=2.9.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah this one is leftover, as we already need psycopg2-binary for other part of lcore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, psycopg2 is already in the main dependency list, this removal has no effect.
|
/retest |
radofuchs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
remove unused dependency from API agents: inline::meta-reference
RAG and MCP server functionalities are not affected.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.