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

fix: un-paralellize pypi actions #1144

Merged
merged 4 commits into from
Jan 7, 2025
Merged

fix: un-paralellize pypi actions #1144

merged 4 commits into from
Jan 7, 2025

Conversation

tushar-composio
Copy link
Contributor

@tushar-composio tushar-composio commented Jan 2, 2025

🔍 Review Summary

Purpose

  • Refine GitHub Actions workflow for Python package releases, improve system compatibility, and enhance process optimization.

Changes

  • Refactor: Modified .github/workflows/release.yaml to remove parallel execution, added conditional checks for release events, and updated macOS version from 12 to 13 in matrix configuration.

Impact

  • Ensures sequential task processing, improved system compatibility, and efficient triggering of release events, enhancing user experience and streamlining the release process.
Original Description

🔍 Review Summary

Purpose: Enhance efficiency, compatibility, and responsiveness of the GitHub Actions workflow for Python packages and plugins.

Changes:

  • Enhancement: Transitioned to a sequential workflow process, removing parallel execution to reduce conflicts. Introduced conditional checks to activate workflows only during release events.

  • Compatibility: Updated macOS version from 12 to 13 in the matrix strategy to meet current system requirements.

Impact: These updates ensure smoother operations, reduced conflicts, and improved workflow responsiveness during release events, while maintaining system compatibility.

Original Description

🔍 Review Summary

Purpose: Enhance the reliability and simplicity of the Python package release process using GitHub Actions.

Changes:

  • Refactor: Removed parallel execution and matrix configuration from .github/workflows/release.yaml to streamline the workflow.

Impact: Consolidates plugin builds into a single step, reducing errors and tailoring the workflow to respond solely to release events, thereby increasing reliability.

Original Description

[!IMPORTANT]
Refactor GitHub Actions workflow to remove parallel execution, update macOS version, and ensure tasks run only on release events.

  • Workflow Changes:
    • Remove matrix strategy and max-parallel settings in .github/workflows/release.yaml.
    • Set runs-on to ubuntu-latest for all jobs.
    • Add if: github.event_name == 'release' to ensure steps run only on release events.
  • Job Modifications:
    • Consolidate plugin builds into a single loop in publish-plugins.
    • Remove needs: publish-core from publish-plugins, publish-swe, and publish-cli jobs.
    • Update publish-cli to conditionally upload artifacts based on OS and release event.
  • Misc:
    • Update macOS version from 12 to 13 in matrix configuration.

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


Copy link

vercel bot commented Jan 2, 2025

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 Jan 7, 2025 9:08am

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 da4e9ae in 32 seconds

More details
  • Looked at 225 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .github/workflows/release.yaml:107
  • Draft comment:
    Removing the needs: publish-core dependency might cause issues if publish-swe requires artifacts or outputs from publish-core. Ensure that publish-swe can run independently or adjust dependencies accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. .github/workflows/release.yaml:215
  • Draft comment:
    Removing the needs: publish-core dependency might cause issues if publish-images requires artifacts or outputs from publish-core. Ensure that publish-images can run independently or adjust dependencies accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_UMiAGlLhDC52dzwp


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

Copy link

Walkthrough

The update refines the GitHub Actions workflow for Python package releases by eliminating parallel execution and simplifying build and publish steps. It ensures the process is triggered only on release events, enhancing reliability. Plugin builds are consolidated into a single step, reducing complexity and potential errors.

Changes

File(s) Summary
.github/workflows/release.yaml Removed parallel execution and matrix configuration. Simplified build and publish steps. Added conditional checks for release events. Consolidated plugin builds into a single loop.

🔗 Related PRs

Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below

Emoji Descriptions:

  • ⚠️ Potential Issue - May require further investigation.
  • 🔒 Security Vulnerability - Fix to ensure system safety.
  • 💻 Code Improvement - Suggestions to enhance code quality.
  • 🔨 Refactor Suggestion - Recommendations for restructuring code.
  • ℹ️ Others - General comments and information.

Interact with the Bot:

  • Send a message or request using the format:
    @bot + *your message*
Example: @bot Can you suggest improvements for this code?
  • Help the Bot learn by providing feedback on its responses.
    @bot + *feedback*
Example: @bot Do not comment on `save_auth` function !

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 fb10ed1 in 25 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/workflows/release.yaml:113
  • Draft comment:
    The matrix includes macos-12, but there are no conditional upload steps for this OS version. Consider removing macos-12 from the matrix if it's not needed.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_rJj9qs2BWUxpEh6j


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

Copy link

Walkthrough

The update to the GitHub Actions workflow for releasing Python packages and plugins focuses on enhancing efficiency and compatibility. By removing parallel execution strategies, the workflow now processes sequentially, reducing potential conflicts. Conditional checks ensure actions are triggered only during release events, optimizing the workflow's responsiveness. Additionally, updating the macOS version in the matrix strategy from 12 to 13 aligns with current system requirements, ensuring smooth operation.

Changes

File(s) Summary
.github/workflows/release.yaml Removed parallel execution strategies, added conditional checks for release events, and updated macOS version from 12 to 13 for compatibility.

🔗 Related PRs

Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below

Emoji Descriptions:

  • ⚠️ Potential Issue - May require further investigation.
  • 🔒 Security Vulnerability - Fix to ensure system safety.
  • 💻 Code Improvement - Suggestions to enhance code quality.
  • 🔨 Refactor Suggestion - Recommendations for restructuring code.
  • ℹ️ Others - General comments and information.

Interact with the Bot:

  • Send a message or request using the format:
    @bot + *your message*
Example: @bot Can you suggest improvements for this code?
  • Help the Bot learn by providing feedback on its responses.
    @bot + *feedback*
Example: @bot Do not comment on `save_auth` function !

mkdir plugins_dist
for plugin in $plugins; do
cd plugins/$plugin
python -m build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding error handling and logging for the plugin build loop. If one plugin fails to build, it could silently fail and continue with others. Adding try-catch with proper error logging would help in debugging issues.

@shreysingla11
Copy link
Collaborator

Review Summary

Changes Overview

  • ✅ Simplified build process by removing matrix strategy and parallel execution
  • ✅ Updated macOS runner from version 12 to 13 (deprecation fix)
  • ✅ Added proper release conditions with if: github.event_name == 'release'
  • ✅ Consolidated plugin publishing into a single job

Code Quality: 8/10

Pros:

  • Improved build stability by removing parallel execution
  • Better release conditions handling
  • Platform support maintained with updated macOS version
  • Good use of secrets and specific action versions

Suggestions for Improvement:

  1. Add error handling for plugin build loop
  2. Add comments explaining architectural decisions
  3. Fix typo in PR title ("un-paralellize" → "un-parallelize")

Overall, the changes improve the release workflow's stability and maintainability. The code is well-structured and follows good practices. Consider implementing the suggested improvements for better error handling and documentation.

Copy link

Walkthrough

This update refines the GitHub Actions workflow for Python package releases by ensuring sequential execution, optimizing triggers for release events, and updating the macOS version to maintain system compatibility.

Changes

File(s) Summary
.github/workflows/release.yaml Removed parallel execution strategies, added conditional checks for release events, and updated macOS version from 12 to 13 in matrix configuration for sequential processing and compatibility.

🔗 Related PRs

Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below

Emoji Descriptions:

  • ⚠️ Potential Issue - May require further investigation.
  • 🔒 Security Vulnerability - Fix to ensure system safety.
  • 💻 Code Improvement - Suggestions to enhance code quality.
  • 🔨 Refactor Suggestion - Recommendations for restructuring code.
  • ℹ️ Others - General comments and information.

Interact with the Bot:

  • Send a message or request using the format:
    @bot + *your message*
Example: @bot Can you suggest improvements for this code?
  • Help the Bot learn by providing feedback on its responses.
    @bot + *feedback*
Example: @bot Do not comment on `save_auth` function !

Comment on lines 139 to 146
cd dist/
zip -r composio-linux-amd64.zip bin/*
rm -rf bin/
- if: matrix.os == 'ubuntu-latest'
- if: matrix.os == 'ubuntu-latest' && github.event_name == 'release'
name: Upload artifact
uses: softprops/action-gh-release@v2
with:
files: python/dist/composio-linux-amd64.zip

Choose a reason for hiding this comment

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

Bug Fix: Review and adjust the artifact upload condition to ensure it triggers for all necessary events, not just 'release'.

🔧 Suggested Code Diff:
- if: matrix.os == 'ubuntu-latest' && github.event_name == 'release'
+ if: matrix.os == 'ubuntu-latest' && (github.event_name == 'release' || github.event_name == 'push')
📝 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.

Suggested change
cd dist/
zip -r composio-linux-amd64.zip bin/*
rm -rf bin/
- if: matrix.os == 'ubuntu-latest'
- if: matrix.os == 'ubuntu-latest' && github.event_name == 'release'
name: Upload artifact
uses: softprops/action-gh-release@v2
with:
files: python/dist/composio-linux-amd64.zip
- if: matrix.os == 'ubuntu-latest' && (github.event_name == 'release' || github.event_name == 'push')
name: Upload artifact
uses: softprops/action-gh-release@v2
with:
# Add any necessary configuration options here
- if: matrix.os == 'macos-12'

- name: Publish ${{ matrix.package }} to PyPI
pip install build twine

plugins="autogen camel claude crew_ai griptape julep langchain llamaindex lyzr openai praisonai langgraph phidata google"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider extracting the plugins list into a configuration file (e.g., plugins.json) to make it easier to maintain and update the list of plugins without modifying the workflow file.

permissions: write-all
strategy:
matrix:
os: [ubuntu-latest, macos-12, macos-14, windows-latest]
os: [ubuntu-latest, macos-13, macos-14, windows-latest]
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are still references to macos-12 in the conditional statements below (lines 149-162) that need to be updated to macos-13 for consistency with this change.

@shreysingla11
Copy link
Collaborator

Review Summary

The changes to un-parallelize PyPI actions and update macOS runners have been reviewed. Here's the overall assessment:

Positive Changes

  • ✅ Simplified plugin publishing process by removing matrix strategy
  • ✅ Added proper release event conditions to prevent unnecessary builds
  • ✅ Updated to macOS-13 runner from deprecated macOS-12
  • ✅ Improved artifact collection with centralized plugins_dist directory

Areas of Concern

  • ⚠️ Removal of job dependencies (needs: publish-core) could lead to race conditions
  • ⚠️ Inconsistent macOS version references in conditional statements
  • ⚠️ Hardcoded plugins list in workflow file could be harder to maintain

Suggestions for Improvement

  1. Consider keeping job dependencies or add verification steps
  2. Move plugins list to a configuration file
  3. Update all remaining macOS-12 references to macOS-13
  4. Add workflow version and last updated comments for better tracking

Overall Code Quality Rating: 7/10

  • Good improvements in workflow structure
  • Needs attention to dependency management and consistency
  • Documentation could be enhanced

Copy link

@aiswaryasankar aiswaryasankar left a comment

Choose a reason for hiding this comment

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

Automated Code Review Findings

@@ -43,6 +39,7 @@ jobs:
pip install twine build
python -m build
- name: Publish Core to PyPI

Choose a reason for hiding this comment

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

HIGH STATE_MANAGEMENT

The release workflow now only publishes packages if the event name is 'release'. Previously, it would attempt to publish on other events (e.g., pushes to branches). This could lead to unintended package publishing and potential versioning conflicts. The fix is to ensure that the if: github.event_name == 'release' condition is correctly applied to all publishing steps.

Comment on lines +64 to +74
pip install build twine

plugins="autogen camel claude crew_ai griptape julep langchain llamaindex lyzr openai praisonai langgraph phidata google"
mkdir plugins_dist
for plugin in $plugins; do
cd plugins/$plugin
python -m build
mv dist/* ../../plugins_dist/
cd ../..
done
- name: Publish plugins to PyPI

Choose a reason for hiding this comment

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

MEDIUM CRASH

The plugin build process has been significantly altered. Previously, plugins were built individually in a matrix. Now, all plugins are built in a loop. While functionally equivalent, this change lacks error handling. If the build of one plugin fails, the subsequent plugins won't be built, leading to an incomplete release. Add error handling (e.g., set -e) to stop the script on failure and provide more informative error messages.

Comment on lines +113 to 114
os: [ubuntu-latest, macos-13, macos-14, windows-latest]
runs-on: ${{ matrix.os }}

Choose a reason for hiding this comment

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

MEDIUM CRASH

The CLI build process now uses a hardcoded list of operating systems for building and uploading artifacts. The removal of macos-12 and the addition of macos-13 might break compatibility with older macOS versions. This should be reviewed to ensure that the supported macOS versions align with the project's requirements and that appropriate testing is in place for the new version.

Copy link

@aiswaryasankar aiswaryasankar left a comment

Choose a reason for hiding this comment

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

Automated Code Review Findings

@@ -43,6 +39,7 @@ jobs:
pip install twine build
python -m build
- name: Publish Core to PyPI

Choose a reason for hiding this comment

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

MEDIUM STATE_MANAGEMENT

The release workflow now only publishes packages when the event name is 'release'. Previously, it may have published on other events (e.g., pushes to the main branch). This change prevents accidental publishing of non-release versions to PyPI, improving reliability and preventing potential confusion for users.

steps:
- uses: actions/checkout@v4
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
- name: Build ${{ matrix.package }}
python-version: "3.10"

Choose a reason for hiding this comment

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

MEDIUM CRASH

The plugin build process has been significantly altered. Previously, plugins were built individually in a matrix. Now, all plugins are built in a loop, potentially leading to build failures cascading and halting the entire process. Error handling is missing. The previous matrix approach allowed for parallel builds and individual failure reporting.

Comment on lines 112 to +113
matrix:
os: [ubuntu-latest, macos-12, macos-14, windows-latest]
os: [ubuntu-latest, macos-13, macos-14, windows-latest]

Choose a reason for hiding this comment

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

LOW CRASH

The macOS version numbers in the CLI build matrix have been updated. While not a bug in itself, the change from macos-12 to macos-13 might cause unexpected behavior if the code relies on specific macOS 12 features. This should be verified.

Copy link

@aiswaryasankar aiswaryasankar left a comment

Choose a reason for hiding this comment

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

Automated Code Review Findings

Copy link

@aiswaryasankar aiswaryasankar left a comment

Choose a reason for hiding this comment

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

Automated Code Review Findings

Copy link

@aiswaryasankar aiswaryasankar left a comment

Choose a reason for hiding this comment

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

Automated Code Review Findings

Copy link

@aiswaryasankar aiswaryasankar left a comment

Choose a reason for hiding this comment

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

Automated Code Review Findings

.github/workflows/release.yaml Show resolved Hide resolved
Copy link

@aiswaryasankar aiswaryasankar left a comment

Choose a reason for hiding this comment

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

Automated Code Review Findings

.github/workflows/release.yaml Show resolved Hide resolved
Comment on lines 234 to +237
run: |
npm install -g @e2b/cli@latest
- name: Publishing Template
if: github.event_name == 'release'

Choose a reason for hiding this comment

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

The release workflow now only publishes packages when the event name is 'release'. Previously, it may have published on other events, potentially leading to unintended deployments to PyPI. To revert to the previous behavior (publishing regardless of event), remove the if: github.event_name == 'release' conditions added in lines 41, 75, 100, 133, 142, 149, 158, 165, 174, 181, 190, 218, 237.

Comment on lines 59 to 80
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
- name: Build ${{ matrix.package }}
python-version: "3.10"
- name: Build plugins
run: |
pip install twine build

# Build dist
cd plugins/${{ matrix.package }}
python -m build
- name: Publish ${{ matrix.package }} to PyPI
pip install build twine

plugins="autogen camel claude crew_ai griptape julep langchain llamaindex lyzr openai praisonai langgraph phidata google"
mkdir plugins_dist
for plugin in $plugins; do
cd plugins/$plugin
python -m build
mv dist/* ../../plugins_dist/
cd ../..
done
- name: Publish plugins to PyPI
if: github.event_name == 'release'
uses: pypa/gh-action-pypi-publish@release/v1
with:
user: ${{ secrets.PYPI_USERNAME }}
password: ${{ secrets.PYPI_PASSWORD }}
skip-existing: true

Choose a reason for hiding this comment

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

The plugin build process has changed to build all plugins in a loop instead of using a matrix. While functionally equivalent, this removes the parallel build capability of the matrix, potentially increasing build time significantly. To restore parallel builds, revert to the original matrix-based approach (lines 59-80).

@@ -167,14 +139,14 @@ jobs:
cd dist/
zip -r composio-linux-amd64.zip bin/*
rm -rf bin/

Choose a reason for hiding this comment

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

The macOS version used in the CLI build matrix has changed from macos-12 to macos-13. While macos-13 is likely a more up-to-date and supported version, this change might introduce compatibility issues with older macOS versions. Consider adding a check to ensure compatibility or reverting to macos-12 if backward compatibility is critical.

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.

❌ Changes requested. Incremental review on 0ae2b05 in 1 minute and 1 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/workflows/release.yaml:158
  • Draft comment:
    The matrix.os value should be 'macos-13' instead of 'macos013'.
      - if: matrix.os == 'macos-13' && github.event_name == 'release'
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_1qQgOTv1062I8QGT


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

name: Upload artifact
uses: softprops/action-gh-release@v2
with:
files: python/dist/composio-linux-amd64.zip

# Mac Intel Release
- if: matrix.os == 'macos-12'
- if: matrix.os == 'macos013' && github.event_name == 'release'
Copy link
Contributor

Choose a reason for hiding this comment

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

The matrix.os value should be 'macos-13' instead of 'macos013'.

Suggested change
- if: matrix.os == 'macos013' && github.event_name == 'release'
- if: matrix.os == 'macos-13' && github.event_name == 'release'

Comment on lines 39 to 45
pip install twine build
python -m build
- name: Publish Core to PyPI
if: github.event_name == 'release'
uses: pypa/gh-action-pypi-publish@release/v1
with:
user: ${{ secrets.PYPI_USERNAME }}

Choose a reason for hiding this comment

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

Code Review: The code changes introduce a condition to ensure that the publishing step to PyPI only occurs during a release event. This is consistent with best practices for CI/CD workflows, ensuring that actions are only executed when appropriate. No issues or conflicts were identified in this section of the code.


Comment on lines 107 to 114

publish-cli:
name: Build and upload CLI
needs: publish-core
permissions: write-all
strategy:
matrix:
os: [ubuntu-latest, macos-12, macos-14, windows-latest]
os: [ubuntu-latest, macos-13, macos-14, windows-latest]
runs-on: ${{ matrix.os }}

Choose a reason for hiding this comment

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

Potential Issue: The removal of the needs: publish-core line changes the dependency structure of the workflow. This could lead to the publish-cli job starting before the publish-core job completes, potentially causing issues if publish-cli relies on artifacts or outputs from publish-core.

Actionable Steps:

  • Evaluate if publish-cli truly does not depend on publish-core. If it does, reintroduce the needs: publish-core line to ensure proper execution order.
  • If the dependency is no longer required, ensure that all necessary artifacts are available for publish-cli to function independently.

This change should be carefully considered to avoid unintended disruptions in the release process. 🛠️

🔧 Suggested Code Diff:
  publish-cli:
    name: Build and upload CLI
+   needs: publish-core
    permissions: write-all
    strategy:
      matrix:
        os: [ubuntu-latest, macos-13, macos-14, windows-latest]
    runs-on: ${{ matrix.os }}
📝 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.

Suggested change
publish-cli:
name: Build and upload CLI
needs: publish-core
permissions: write-all
strategy:
matrix:
os: [ubuntu-latest, macos-12, macos-14, windows-latest]
os: [ubuntu-latest, macos-13, macos-14, windows-latest]
runs-on: ${{ matrix.os }}
publish-cli:
name: Build and upload CLI
needs: publish-core
permissions: write-all
strategy:
matrix:
os: [ubuntu-latest, macos-13, macos-14, windows-latest]
runs-on: ${{ matrix.os }}

Comment on lines 130 to 136
pyinstaller composio/cli/__main__.py

# Ubuntu Release
- if: matrix.os == 'ubuntu-latest'
- if: matrix.os == 'ubuntu-latest' && github.event_name == 'release'
run: |
cd python/
mv dist/__main__ dist/bin

Choose a reason for hiding this comment

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

Refactor: The code change ensures that the release-specific actions are only executed during actual release events on the 'ubuntu-latest' OS. This update maintains logical consistency across the workflow by aligning with similar conditions elsewhere in the file. This is a good practice as it prevents unintended execution of release steps during non-release events, which could lead to errors or unnecessary operations.


Comment on lines 155 to 162
cd dist/
zip -r composio-darwin-amd64.zip bin/*
rm -rf bin/
- if: matrix.os == 'macos-12'
- if: matrix.os == 'macos013' && github.event_name == 'release'
name: Upload artifact
uses: softprops/action-gh-release@v2
with:
files: python/dist/composio-darwin-amd64.zip

Choose a reason for hiding this comment

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

Potential Issue: The condition matrix.os == 'macos013' appears to be a typo. It should likely be matrix.os == 'macos-13' to align with the matrix configuration. This typo could prevent the correct execution of the upload step for macOS builds, potentially leading to missing artifacts in the release process.

🔧 Suggested Code Diff:
- if: matrix.os == 'macos013' && github.event_name == 'release'
+ if: matrix.os == 'macos-13' && github.event_name == 'release'
📝 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.

Suggested change
cd dist/
zip -r composio-darwin-amd64.zip bin/*
rm -rf bin/
- if: matrix.os == 'macos-12'
- if: matrix.os == 'macos013' && github.event_name == 'release'
name: Upload artifact
uses: softprops/action-gh-release@v2
with:
files: python/dist/composio-darwin-amd64.zip
- if: matrix.os == 'macos-13' && github.event_name == 'release'
name: Upload artifact
uses: softprops/action-gh-release@v2
with:
# Add necessary parameters for the action here
files: composio-darwin-amd64.zip
token: ${{ secrets.GITHUB_TOKEN }}

Comment on lines 171 to 178
cd dist/
zip -r composio-darwin-arm64.zip bin/*
rm -rf bin/
- if: matrix.os == 'macos-14'
- if: matrix.os == 'macos-14' && github.event_name == 'release'
name: Upload artifact
uses: softprops/action-gh-release@v2
with:
files: python/dist/composio-darwin-arm64.zip

Choose a reason for hiding this comment

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

Refactor: The change introduces a condition to ensure that the artifact is only uploaded during a 'release' event for macOS 14. This aligns with the workflow's intent to prevent unnecessary uploads during non-release events, maintaining consistency across different OS-specific steps. This is a good practice to optimize workflow runs and resource usage.


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 6463d7d in 12 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/workflows/release.yaml:149
  • Draft comment:
    The condition matrix.os == 'macos-13' was corrected from macos013. Ensure all matrix.os conditions are consistent and correct. This change is also applied on line 158.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_vdOLizD9exltjaHD


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

Comment on lines 107 to 114

publish-cli:
name: Build and upload CLI
needs: publish-core
permissions: write-all
strategy:
matrix:
os: [ubuntu-latest, macos-12, macos-14, windows-latest]
os: [ubuntu-latest, macos-13, macos-14, windows-latest]
runs-on: ${{ matrix.os }}

Choose a reason for hiding this comment

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

Potential Issue: The removal of the needs: publish-core line changes the dependency structure of the workflow. This could lead to the publish-cli step executing before the publish-core step, potentially causing issues if publish-cli relies on artifacts or outputs from publish-core.

Actionable Steps:

  • Evaluate if publish-cli truly does not depend on publish-core. If it does, reintroduce the needs: publish-core line to maintain the correct execution order.
  • If the dependency is no longer required, ensure that all necessary artifacts or outputs are available for publish-cli to function correctly without publish-core.
🔧 Suggested Code Diff:
 publish-cli:
     name: Build and upload CLI
+    needs: publish-core
     permissions: write-all
     strategy:
       matrix:
         os: [ubuntu-latest, macos-13, macos-14, windows-latest]
📝 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.

Suggested change
publish-cli:
name: Build and upload CLI
needs: publish-core
permissions: write-all
strategy:
matrix:
os: [ubuntu-latest, macos-12, macos-14, windows-latest]
os: [ubuntu-latest, macos-13, macos-14, windows-latest]
runs-on: ${{ matrix.os }}
publish-cli:
name: Build and upload CLI
needs: publish-core
permissions: write-all
strategy:
matrix:
os: [ubuntu-latest, macos-13, macos-14, windows-latest]

Comment on lines 130 to 136
pyinstaller composio/cli/__main__.py

# Ubuntu Release
- if: matrix.os == 'ubuntu-latest'
- if: matrix.os == 'ubuntu-latest' && github.event_name == 'release'
run: |
cd python/
mv dist/__main__ dist/bin

Choose a reason for hiding this comment

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

Code Improvement: The code changes improve the logical consistency of the workflow by ensuring that the release-specific actions are only executed during actual release events. This is achieved by adding a condition to check the event type (github.event_name == 'release'). This change aligns with similar modifications elsewhere in the file, enhancing the overall clarity and maintainability of the workflow configuration.

  • Pros:

    • Ensures actions are only performed during release events, preventing unnecessary executions.
    • Maintains consistency across different operating systems.
  • Considerations:

    • Ensure that the dist/__main__ path is correct and that the dist/bin directory exists or is created as needed to avoid runtime errors.

Comment on lines 155 to 162
cd dist/
zip -r composio-darwin-amd64.zip bin/*
rm -rf bin/
- if: matrix.os == 'macos-12'
- if: matrix.os == 'macos-13' && github.event_name == 'release'
name: Upload artifact
uses: softprops/action-gh-release@v2
with:
files: python/dist/composio-darwin-amd64.zip

Choose a reason for hiding this comment

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

Potential Issue: The change from 'macos-12' to 'macos-13' aligns with the matrix definition update, ensuring consistency. However, the introduction of 'macos-14' in the condition seems to be an oversight, as it is not reflected in the matrix definition. This could lead to unexpected behavior if 'macos-14' is not supported or defined elsewhere in the workflow.

🔧 Suggested Code Diff:
-      - if: matrix.os == 'macos-14'
📝 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.

Suggested change
cd dist/
zip -r composio-darwin-amd64.zip bin/*
rm -rf bin/
- if: matrix.os == 'macos-12'
- if: matrix.os == 'macos-13' && github.event_name == 'release'
name: Upload artifact
uses: softprops/action-gh-release@v2
with:
files: python/dist/composio-darwin-amd64.zip
cd dist/
zip -r composio-darwin-amd64.zip bin/*
rm -rf bin/
- if: matrix.os == 'macos-13' && github.event_name == 'release'
name: Upload artifact
uses: softprops/action-gh-release@v2
with:

Comment on lines 215 to 221
with:
python-version: "3.10"
- name: Publishing tool server images
if: github.event_name == 'release'
run: |
cd python/
pip3 install .

Choose a reason for hiding this comment

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

Code Improvement: The addition of the condition if: github.event_name == 'release' ensures that the step for publishing tool server images is only executed during a release event. This change aligns with similar conditions applied elsewhere in the workflow, maintaining logical consistency and preventing unnecessary execution of steps outside of release events. This is a good practice for optimizing workflow efficiency and clarity. No further action is required.


@tushar-composio tushar-composio merged commit 1c0019b into master Jan 7, 2025
10 of 11 checks passed
@tushar-composio tushar-composio deleted the ENG-2540 branch January 7, 2025 09:28
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.

4 participants