-
Notifications
You must be signed in to change notification settings - Fork 2
fix: Permissions and pymilvus conflict #7
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
Reviewer's GuideThis PR restructures Docker build processes and Python dependency management to resolve container permission issues and address pymilvus version conflicts. It reorders user contexts in containerfiles, unpins pymilvus across configurations, updates GitHub Actions tagging, and corrects registry paths. Class diagram for ProviderSpec and AdapterSpec pymilvus dependency changeclassDiagram
class ProviderSpec {
+Api api
+AdapterSpec adapter_spec
}
class AdapterSpec {
+adapter_type: str
+pip_packages: list[str] // Now ["pymilvus"]
+module: str
+config_class: str
+description: str
}
ProviderSpec --> AdapterSpec
class InlineProviderSpec {
+api: Api
+provider_type: str
+pip_packages: list[str] // Now ["pymilvus"]
+module: str
+config_class: str
+api_dependencies: list[Api]
}
ProviderSpec --> InlineProviderSpec
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @ruivieira - I've reviewed your changes - here's some feedback:
- Consider consolidating the USER root and directory permission logic into a single build step or helper to reduce duplication and ensure consistent permissions before switching to the non-root user.
- Unpinning pymilvus entirely could lead to unexpected breaking changes; consider specifying a minimum or maximum version range (or using a constraints file) to maintain compatibility.
- In the GitHub Actions workflow, confirm that the docker tag commands align with the actual image tags produced by the build step to avoid tagging mismatches.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider consolidating the USER root and directory permission logic into a single build step or helper to reduce duplication and ensure consistent permissions before switching to the non-root user.
- Unpinning pymilvus entirely could lead to unexpected breaking changes; consider specifying a minimum or maximum version range (or using a constraints file) to maintain compatibility.
- In the GitHub Actions workflow, confirm that the docker tag commands align with the actual image tags produced by the build step to avoid tagging mismatches.
## Individual Comments
### Comment 1
<location> `trustyai-distribution/Containerfile:33` </location>
<code_context>
pillow \
psycopg2-binary \
- pymilvus>=2.4.10 \
+ pymilvus \
pymongo \
pypdf \
</code_context>
<issue_to_address>
Removing the version pin for pymilvus may introduce compatibility issues.
Installing the latest pymilvus version may cause unexpected issues. Pin to a compatible version range if possible to maintain stability.
</issue_to_address>
### Comment 2
<location> `llama_stack/providers/registry/vector_io.py:524` </location>
<code_context>
AdapterSpec(
adapter_type="milvus",
- pip_packages=["pymilvus>=2.4.10"],
+ pip_packages=["pymilvus"],
module="llama_stack.providers.remote.vector_io.milvus",
config_class="llama_stack.providers.remote.vector_io.milvus.MilvusVectorIOConfig",
</code_context>
<issue_to_address>
Unpinned pymilvus dependency may cause unexpected behavior.
Without a minimum version, environments may install incompatible pymilvus versions. Specify a minimum version if your code depends on certain features or fixes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pillow \ | ||
| psycopg2-binary \ | ||
| pymilvus>=2.4.10 \ | ||
| pymilvus \ |
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.
issue (bug_risk): Removing the version pin for pymilvus may introduce compatibility issues.
Installing the latest pymilvus version may cause unexpected issues. Pin to a compatible version range if possible to maintain stability.
| AdapterSpec( | ||
| adapter_type="milvus", | ||
| pip_packages=["pymilvus>=2.4.10"], | ||
| pip_packages=["pymilvus"], |
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.
issue (bug_risk): Unpinned pymilvus dependency may cause unexpected behavior.
Without a minimum version, environments may install incompatible pymilvus versions. Specify a minimum version if your code depends on certain features or fixes.
saichandrapandraju
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, Thanks @ruivieira !
b7c2636
into
trustyai-explainability:main
Summary by Sourcery
Fix container permission issues by ensuring root context during setup and switching back to a non-root user, remove strict pymilvus version constraints to resolve dependency conflicts, and enhance Docker build workflow and base image references.
Bug Fixes:
Build:
CI: