C7: move LLM catalogue and config to praisonai-code#2549
Conversation
Model listing and AutoGen config_list building no longer import the wrapper; shims preserve praisonai.llm.* module identity for backward compatibility.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more β On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the βοΈ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
β¨ Finishing Touchesπ§ͺ Generate unit tests (beta)
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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first β incorporate their findings. Review areas:
|
There was a problem hiding this comment.
Code Review
This pull request moves the implementation of ModelCatalogue and build_config_list from praisonai to praisonai_code to support standalone usage, replacing the old locations with backward-compatibility shims. Feedback on the changes suggests correcting a falsy check on model costs in catalogue.py to prevent zero-cost values from being ignored, and optimizing validate_model to avoid redundant calls to describe_model.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| input_cost=info.get("input_cost_per_token") * 1000 if info.get("input_cost_per_token") else None, | ||
| output_cost=info.get("output_cost_per_token") * 1000 if info.get("output_cost_per_token") else None, |
There was a problem hiding this comment.
When retrieving the model cost from litellm, checking if info.get("input_cost_per_token") will treat a cost of 0 (which is common for free or local models) as falsy, causing it to fall back to None. Use an explicit is not None check to correctly preserve zero-cost values.
| input_cost=info.get("input_cost_per_token") * 1000 if info.get("input_cost_per_token") else None, | |
| output_cost=info.get("output_cost_per_token") * 1000 if info.get("output_cost_per_token") else None, | |
| input_cost=info.get("input_cost_per_token") * 1000 if info.get("input_cost_per_token") is not None else None, | |
| output_cost=info.get("output_cost_per_token") * 1000 if info.get("output_cost_per_token") is not None else None, |
| if self.is_valid_model(model_id): | ||
| # Return the correctly cased version | ||
| info = self.describe_model(model_id) | ||
| if info: | ||
| return info["id"] | ||
| return model_id |
There was a problem hiding this comment.
In validate_model, calling self.is_valid_model(model_id) followed by self.describe_model(model_id) results in redundant passes over the models list (since is_valid_model internally calls describe_model). You can optimize this by calling describe_model once and checking if the result is not None.
| if self.is_valid_model(model_id): | |
| # Return the correctly cased version | |
| info = self.describe_model(model_id) | |
| if info: | |
| return info["id"] | |
| return model_id | |
| info = self.describe_model(model_id) | |
| if info: | |
| return info["id"] |
Greptile SummaryThis PR moves the LLM catalogue and config helpers into
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(c7): correct zero-cost handling, ded..." | Re-trigger Greptile |
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task ββ View job Final Architecture Review β PR #2549 (C7: LLM catalogue & config)Verdict: β
Approve (all valid reviewer findings fixed and pushed to Phase 1 β Architecture (AGENTS.md)
Phase 2 β Fixes implemented
Files modified:
Skipped / N/A:
Phase 3 β TestsAlso verified:
Job run Β· Branch |
β¦__all__ - catalogue.py: use 'is not None' so zero-cost (free/local) models keep 0.0 cost instead of falling back to None (Gemini) - catalogue.py: validate_model now does a single describe_model lookup instead of is_valid_model + describe_model double pass (Gemini) - llm/__init__.py: bind all names advertised in __all__ so 'from praisonai_code.llm import *' and named imports resolve (Greptile) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task ββ View job Final Architecture Review β PR #2549 (C7: LLM catalogue & config)Verdict: β
Approve β all valid reviewer findings are already fixed on this branch (commit Phase 1 β Architecture (AGENTS.md)
Phase 2 β Reviewer findings (all already resolved on branch)
Skipped / N/A: Qodo (reviews paused, no findings) Β· CodeRabbit (auto-review skipped on non-default base, no findings) Β· Copilot (no substantive findings). No security issues (no secrets/eval/exec). Shims, Phase 3 β TestsAlso verified in a fresh interpreter:
No files modified in this run (branch already clean at Job run Β· Branch |
3e6fa0f
into
feat/c7-llm-credentials-standalone
Summary
llm/catalogue.pyandllm/config.pyintopraisonai_codesopraisonai models listand AutoGenbuild_config_listwork without the wrapperenv.pyto load catalogue locally (no_wrapper_bridgehop)modelscommand andmain.py; extend C5 compat tests and CI standalone smokeStacks on: #2548 (
feat/c7-llm-credentials-standalone) β merge that first.Test plan
pytest tests/unit/test_c5_backward_compat.py tests/unit/llm/test_env_resolver.py tests/unit/llm/test_credentials_is_configured.pyβ 90 passedModelCatalogue().list_models(provider="openai")β 132 modelspraisonai.llm.*imports remain inpraisonai-code