Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The [rerank] configuration table still marks api_key as universally required even though it is optional for self-hosted vllm endpoints, leaving the docs inconsistent with the implementation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates EverOS’s configuration reference/docs to include the existing DashScope rerank provider and clarifies API-key expectations in the rerank provider factory documentation so the docs match current behavior.
Changes:
- Document
dashscopeas a supported[rerank].provideralongsidedeepinfraandvllm. - Clarify (docstring) that
api_keyis required fordeepinfraanddashscope, but optional for self-hostedvllmendpoints.
File summaries
| File | Description |
|---|---|
| src/everos/component/rerank/factory.py | Updates the build_rerank_provider docstring to reflect DashScope support and api_key requirements by provider. |
| docs/configuration.md | Expands the [rerank] provider field documentation to include dashscope. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | `provider` | string | `"deepinfra"` | No | Rerank provider: `deepinfra` or `vllm`. | | ||
| | `provider` | string | `"deepinfra"` | No | Rerank provider: `deepinfra`, `vllm`, or `dashscope`. | | ||
| | `model` | string | — | **Yes** | Reranker model identifier. | | ||
| | `api_key` | string | — | **Yes** | API key. | |
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
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
dashscopeas a supported[rerank].provideralongsidedeepinfraandvllm.build_rerank_providerthatapi_keyis required for both DeepInfra and DashScope, while it remains optional for self-hosted vLLM endpoints.Area
Verification
Repo-wide
make ciwas not run because the execution environment could not create a working-tree checkout. The change does not alter runtime behavior.Checklist
main..envfiles, dependency folders, or generated output.Notes for Reviewers
RerankSettingsand the provider factory already support DashScope; this PR only brings the configuration reference and the factory docstring in sync with the existing implementation.By submitting this pull request, I agree that my contribution is licensed under
the Apache License 2.0.