Skip to content
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

chore: use tox-uv to speed up dep install in ci #887

Merged
merged 8 commits into from
Dec 12, 2024
Merged

Conversation

tushar-composio
Copy link
Contributor

@tushar-composio tushar-composio commented Nov 21, 2024

Important

Update CI to use tox-uv for faster dependency installation, adjust tox version range, and add swe test marker.

  • CI Configuration:
    • Update .github/workflows/common.yml to use tox-uv for faster dependency installation.
    • Change tox version to >=4.21,<5 in linter_checks, test, and e2e jobs.
  • Tox Configuration:
    • Modify python/tox.ini to use uv pip install for installing dependencies in testenv:test.
  • Testing:
    • Add swe marker in pytest.ini for SWE tests.
    • Add @pytest.mark.swe to TestIntegration class in test_benchmark.py.

This description was created by Ellipsis for 5ea8826. It will automatically update as commits are pushed.

Copy link

vercel bot commented Nov 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2024 0:47am

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 41dd44c in 8 seconds

More details
  • Looked at 31 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/workflows/common.yml:56
  • Draft comment:
    Ensure that tox-uv is compatible with all the tox environments used in the workflow. This includes black-diff, isort-check, flake8, pylint, and mypy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR updates the tox version and adds tox-uv for faster dependency installation. This change is applied consistently across different jobs in the workflow.

Workflow ID: wflow_mBLYOZjMFO7YfD8D


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

github-actions bot commented Nov 21, 2024

This comment was generated by github-actions[bot]!

JS SDK Coverage Report

📊 Coverage report for JS SDK can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/coverage-11971353188/coverage/index.html

📁 Test report folder can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/html-report-11971353188/html-report/report.html

@shreysingla11
Copy link
Collaborator

Code Review Summary

Changes Overview

  • Replaces tox==4.6.3 with 'tox>=4.21,<5' tox-uv across three CI jobs
  • Introduces tox-uv for faster dependency installation in CI
  • Updates tox version to a more recent range while preventing major version jumps

Positive Aspects

✅ Performance optimization through tox-uv integration
✅ Good version constraint practice (allowing patches but preventing major version jumps)
✅ Consistent changes across all jobs
✅ Low-risk infrastructure improvement

Suggestions

  • Consider adding comments to document the use of tox-uv and version requirements
  • Could be helpful to monitor and document the performance improvement

Overall Assessment

The changes are well-structured and should improve CI pipeline performance. The implementation is clean and follows good practices for version management. Approved with minor documentation suggestions.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 209d84c in 9 seconds

More details
  • Looked at 35 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/tox.ini:130
  • Draft comment:
    Consider uncommenting the uv pip install lines for plugins or removing them if they are not needed. This will ensure consistency and clarity in the configuration.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of uv before pip install commands is consistent with the PR's intent to use tox-uv for faster dependency installation. However, the commented-out lines should be addressed for consistency and clarity.

Workflow ID: wflow_YW7uk1udBckJIcjg


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on a709044 in 10 seconds

More details
  • Looked at 84 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/pytest.ini:4
  • Draft comment:
    Consider adding a description for the 'swe' marker in the pytest.ini file for better clarity and documentation.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a new marker 'swe' in pytest.ini and uses it in test_benchmark.py. However, the 'swe' marker is not documented in the pytest.ini file, which is a best practice for clarity.

Workflow ID: wflow_Yn4ME4NIUk3zqRaQ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 5ea8826 in 12 seconds

More details
  • Looked at 31 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/workflows/common.yml:75
  • Draft comment:
    Removing the 'needs' dependency on 'lock_check' might cause issues if 'lock_check' is required to run before other jobs. Ensure that 'lock_check' is not a prerequisite for the jobs in this workflow.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_Kl1woJh2QqwumVxM


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@tushar-composio tushar-composio merged commit 3f02820 into master Dec 12, 2024
11 checks passed
@tushar-composio tushar-composio deleted the tox-uv branch December 12, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants