-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat: migrate from pipenv to uv package manager #1378
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
python/requirements.txt
Outdated
@@ -0,0 +1,14 @@ | |||
requests>=2.31.0,<3 |
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.
Consider adding a comment block at the top of both requirements files explaining their purpose and usage. This would help new developers understand which dependencies are needed for what purpose.
.github/workflows/common.yml
Outdated
time pipenv run pip install -e .[all] | ||
time pipenv lock | ||
time pipenv run composio --help | ||
pip install uv |
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.
Consider adding error handling for UV installation in case pip install fails. Something like:
if ! pip install uv; then
echo "Failed to install UV package manager"
exit 1
fi
pipenv run pip install -e plugins/openai;\ | ||
pipenv run pip install -e .;\ | ||
echo "Enter virtual environment with all development dependencies now: 'pipenv shell'.";\ | ||
python -m venv .venv;\ |
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.
Consider adding a check for Python version compatibility with UV before installation in the Makefile. UV requires Python 3.8+.
Code Review SummaryThe migration from Pipenv to UV package manager looks well-structured and maintains all existing functionality. Here's a breakdown of the changes: Positive Aspects
Suggestions for Improvement
Overall AssessmentThe changes are well-implemented and maintain the project's functionality while modernizing the package management approach. The migration appears safe to merge after addressing the minor suggestions above. Code Quality Rating: 8/10 |
.github/workflows/common.yml
Outdated
# Install uv with error handling | ||
if ! pip install uv; then | ||
echo "Failed to install UV package manager" | ||
exit 1 | ||
fi | ||
uv pip install -e .[all] | ||
uv pip freeze | ||
uv pip run composio --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.
Error handling is added for uv
install but missing for subsequent uv pip
commands which could also fail. Should add error handling for all critical package installation steps.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
# Install uv with error handling | |
if ! pip install uv; then | |
echo "Failed to install UV package manager" | |
exit 1 | |
fi | |
uv pip install -e .[all] | |
uv pip freeze | |
uv pip run composio --help | |
# Install uv with error handling | |
if ! pip install uv; then | |
echo "Failed to install UV package manager" | |
exit 1 | |
fi | |
# Install packages with error handling | |
if ! uv pip install -e .[all]; then | |
echo "Failed to install required packages" | |
exit 1 | |
fi | |
if ! uv pip freeze; then | |
echo "Failed to freeze dependencies" | |
exit 1 | |
fi | |
if ! uv pip run composio --help; then | |
echo "Failed to run composio help" | |
exit 1 | |
fi |
.github/workflows/common.yml
Outdated
- name: Run tests | ||
run: | | ||
export COMPOSIO_API_KEY=${{ secrets.COMPOSIO_API_KEY_STAGING }} | ||
export COMPOSIO_BASE_URL=${{ secrets.COMPOSIO_BASE_URL_STAGING }} | ||
export FLY_API_TOKEN=${{ secrets.FLY_API_TOKEN }} | ||
export E2B_API_KEY=${{ secrets.E2B_API_KEY_STAGING }} | ||
|
||
tox -e test -- -m 'swe' | ||
- name: Upload test results to Codecov | ||
uses: codecov/test-results-action@v1 | ||
with: | ||
token: ${{ secrets.CODECOV_TOKEN }} | ||
tox -e py |
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.
Removed critical environment variables (COMPOSIO_API_KEY
, FLY_API_TOKEN
, etc.) from test jobs without replacement, which will cause tests requiring these credentials to fail
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
- name: Run tests | |
run: | | |
export COMPOSIO_API_KEY=${{ secrets.COMPOSIO_API_KEY_STAGING }} | |
export COMPOSIO_BASE_URL=${{ secrets.COMPOSIO_BASE_URL_STAGING }} | |
export FLY_API_TOKEN=${{ secrets.FLY_API_TOKEN }} | |
export E2B_API_KEY=${{ secrets.E2B_API_KEY_STAGING }} | |
tox -e test -- -m 'swe' | |
- name: Upload test results to Codecov | |
uses: codecov/test-results-action@v1 | |
with: | |
token: ${{ secrets.CODECOV_TOKEN }} | |
tox -e py | |
- name: Run tests | |
env: | |
COMPOSIO_API_KEY: ${{ secrets.COMPOSIO_API_KEY }} | |
FLY_API_TOKEN: ${{ secrets.FLY_API_TOKEN }} | |
run: | | |
tox -e py |
.github/workflows/common.yml
Outdated
- uses: actions/checkout@v4 | ||
with: | ||
submodules: recursive | ||
fetch-depth: 0 | ||
|
||
- name: Install Docker Engine without containerd | ||
run: | | ||
sudo apt-get update | ||
sudo apt-get install -y apt-transport-https ca-certificates curl software-properties-common | ||
curl -fsSL https://download.docker.com/linux/ubuntu/gpg | sudo apt-key add - | ||
sudo add-apt-repository "deb [arch=amd64] https://download.docker.com/linux/ubuntu $(lsb_release -cs) stable" | ||
sudo apt-get update | ||
sudo apt-get install -y docker-ce docker-ce-cli | ||
sudo systemctl start docker | ||
sudo systemctl enable docker | ||
|
||
- uses: actions/setup-python@v5 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
- name: Install Dependencies | ||
run: | | ||
pip install 'tox>=4.21,<5' tox-uv | ||
- name: Build docker images | ||
- name: Install dependencies | ||
run: | | ||
cd dockerfiles | ||
make ci | ||
- name: Unittests | ||
pip install tox | ||
- name: Run tests | ||
run: | | ||
export COMPOSIO_API_KEY=${{ secrets.COMPOSIO_API_KEY_STAGING }} | ||
export COMPOSIO_BASE_URL=${{ secrets.COMPOSIO_BASE_URL_STAGING }} | ||
export FLY_API_TOKEN=${{ secrets.FLY_API_TOKEN }} | ||
export E2B_API_KEY=${{ secrets.E2B_API_KEY_STAGING }} | ||
|
||
tox -e test -- -m 'e2e' | ||
tox -e e2e |
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.
Removed Docker setup steps from e2e job which are likely required for end-to-end testing, causing e2e tests that depend on Docker to fail
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
- uses: actions/checkout@v4 | |
with: | |
submodules: recursive | |
fetch-depth: 0 | |
- name: Install Docker Engine without containerd | |
run: | | |
sudo apt-get update | |
sudo apt-get install -y apt-transport-https ca-certificates curl software-properties-common | |
curl -fsSL https://download.docker.com/linux/ubuntu/gpg | sudo apt-key add - | |
sudo add-apt-repository "deb [arch=amd64] https://download.docker.com/linux/ubuntu $(lsb_release -cs) stable" | |
sudo apt-get update | |
sudo apt-get install -y docker-ce docker-ce-cli | |
sudo systemctl start docker | |
sudo systemctl enable docker | |
- uses: actions/setup-python@v5 | |
with: | |
python-version: ${{ matrix.python-version }} | |
- name: Install Dependencies | |
run: | | |
pip install 'tox>=4.21,<5' tox-uv | |
- name: Build docker images | |
- name: Install dependencies | |
run: | | |
cd dockerfiles | |
make ci | |
- name: Unittests | |
pip install tox | |
- name: Run tests | |
run: | | |
export COMPOSIO_API_KEY=${{ secrets.COMPOSIO_API_KEY_STAGING }} | |
export COMPOSIO_BASE_URL=${{ secrets.COMPOSIO_BASE_URL_STAGING }} | |
export FLY_API_TOKEN=${{ secrets.FLY_API_TOKEN }} | |
export E2B_API_KEY=${{ secrets.E2B_API_KEY_STAGING }} | |
tox -e test -- -m 'e2e' | |
tox -e e2e | |
- uses: actions/checkout@v4 | |
- uses: actions/setup-python@v5 | |
with: | |
python-version: ${{ matrix.python-version }} | |
- name: Set up Docker | |
uses: docker/setup-buildx-action@v3 | |
- name: Install dependencies | |
run: | | |
pip install tox | |
- name: Run tests | |
run: | | |
tox -e e2e |
…ariables Co-Authored-By: [email protected] <[email protected]>
sudo apt-get install -y docker-ce docker-ce-cli | ||
sudo systemctl start docker | ||
sudo systemctl enable docker | ||
|
||
- uses: actions/setup-python@v5 |
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.
Docker installation commands use deprecated apt-key add
which is a security risk. Use signed-by option with apt-get instead for secure package verification.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
sudo apt-get install -y docker-ce docker-ce-cli | |
sudo systemctl start docker | |
sudo systemctl enable docker | |
- uses: actions/setup-python@v5 | |
- name: Install Docker Engine | |
run: | | |
sudo apt-get update | |
sudo apt-get install -y apt-transport-https ca-certificates curl software-properties-common | |
curl -fsSL https://download.docker.com/linux/ubuntu/gpg | sudo gpg --dearmor -o /usr/share/keyrings/docker-archive-keyring.gpg | |
echo "deb [arch=amd64 signed-by=/usr/share/keyrings/docker-archive-keyring.gpg] https://download.docker.com/linux/ubuntu $(lsb_release -cs) stable" | sudo tee /etc/apt/sources.list.d/docker.list > /dev/null | |
sudo apt-get update | |
sudo apt-get install -y docker-ce docker-ce-cli | |
sudo systemctl start docker | |
sudo systemctl enable docker |
… installation Co-Authored-By: [email protected] <[email protected]>
Migrate from pipenv to uv package manager with pyproject.toml and workspaces
This PR migrates the project from pipenv to the uv package manager, using pyproject.toml instead of requirements.txt files, and implements uv workspaces for the plugins.
Changes
Benefits
Link to Devin run: https://app.devin.ai/sessions/f7646e35f78740cb9f5b8d1d21e7c670
Requested by: [email protected]