Add a Github workflow to test pip/conda package installation #353
Conversation
WalkthroughThe pull request introduces changes to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pyproject.toml (1)
PR objective and changes mismatch
The PR objective states "Add a Github workflow to test pip/conda package installation", but the only change in this PR is a version update in
pyproject.toml. There are no additions or modifications related to GitHub workflows or package installation testing.🔗 Analysis chain
Line range hint
1-124: PR objective and changes mismatchThe PR objective states "Add a Github workflow to test pip/conda package installation", but the only change in this file is a version update. There are no additions or modifications related to GitHub workflows or package installation testing.
To align with the PR objective, consider the following steps:
- Add a
.github/workflowsdirectory if it doesn't exist.- Create a new YAML file in that directory (e.g.,
package_installation_test.yml) to define the GitHub Actions workflow.- In the workflow file, include steps to:
- Set up different Python environments
- Install the package using both pip and conda
- Run basic import or functionality tests
Here's a shell script to check if these steps have been taken:
If the workflow file is not present or if there are no other changes related to the PR objective, please update the PR to include the necessary changes for adding the GitHub workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the existence of the workflow file and its contents if [ -f .github/workflows/package_installation_test.yml ]; then echo "Workflow file exists. Contents:" cat .github/workflows/package_installation_test.yml else echo "Workflow file does not exist in the expected location." fi # Check for any new or modified files in the PR git diff --name-status origin/main...HEADLength of output: 422
Script:
#!/bin/bash # Identify the default branch of the repository default_branch=$(git remote show origin | sed -n '/HEAD branch/s/.*: //p') echo "Default branch is: $default_branch" # Check for the existence of the workflow file and its contents if [ -f .github/workflows/package_installation_test.yml ]; then echo "Workflow file exists. Contents:" cat .github/workflows/package_installation_test.yml else echo "Workflow file does not exist in the expected location." fi # Check for any new or modified files in the PR compared to the default branch if [ -n "$default_branch" ]; then git fetch origin $default_branch git diff --name-status origin/$default_branch...HEAD else echo "Could not determine the default branch." fiLength of output: 529
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- pyproject.toml (1 hunks)
🔇 Additional comments (1)
pyproject.toml (1)
3-3: Version update looks good, but seems unrelated to PR objectives.The version update from "1.7.1" to "1.8.0-pre" is correct and follows semantic versioning practices. However, this change doesn't appear to be directly related to the PR objective of "Add a Github workflow to test pip/conda package installation".
Could you clarify how this version change relates to the addition of a GitHub workflow for testing package installation? If it's not related, consider separating this change into a different PR to maintain clear change history.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
.github/workflows/release.yml (1)
Line range hint
1-116: Consider reordering jobs for a more robust workflow.The addition of test jobs for both Conda and pip installations is excellent for ensuring the package works correctly across different installation methods. However, the current structure runs these tests after the package is published to PyPI.
Consider reordering the jobs to run the tests before deployment. This change would prevent publishing a potentially broken package. Here's a suggested structure:
- Run
test-condajob- Run
test-pipjob- If both tests pass, run the
deployjobYou can achieve this by adding a
needskey to thedeployjob:deploy: needs: [test-conda, test-pip] # ... rest of the deploy job definitionThis ensures that the package is only published if it passes both the Conda and pip installation tests, providing an additional layer of quality assurance.
🧰 Tools
🪛 actionlint
21-21: shellcheck reported issue in this script: SC2086:info:1:31: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: unexpected key "test-conda" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
76-76: unexpected key "test-pip" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/release.yml (2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/release.yml
25-25: unexpected key "test-conda" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
76-76: unexpected key "test-pip" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
🔇 Additional comments (1)
.github/workflows/release.yml (1)
1-1: Workflow name update is appropriate.The updated workflow name accurately reflects the added functionality of testing package installation. This change improves the clarity of the workflow's purpose.
.github/workflows/release.yml
Outdated
| test-conda: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| platform: [linux/amd64] | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up Miniconda | ||
| uses: conda-incubator/setup-miniconda@v2 | ||
| with: | ||
| auto-activate-base: false | ||
| python-version: 3.11 | ||
| miniconda-version: "latest" | ||
| architecture: x86_64 | ||
| activate-environment: test-env | ||
|
|
||
| - name: Create conda environment and install injective-py | ||
| shell: bash -l {0} | ||
| run: | | ||
| conda create -n test-env -y | ||
| conda activate test-env | ||
| conda install -c conda-forge injective-py -y | ||
|
|
||
| - name: Write a Python script | ||
| run: | | ||
| echo " | ||
| import asyncio | ||
|
|
||
| from pyinjective.async_client import AsyncClient | ||
| from pyinjective.core.network import Network | ||
|
|
||
|
|
||
| async def main() -> None: | ||
| network = Network.testnet() | ||
| client = AsyncClient(network) | ||
| latest = await client.fetch_latest_block() | ||
| print(latest) | ||
|
|
||
| asyncio.get_event_loop().run_until_complete(main()) | ||
|
|
||
| " > script.py | ||
|
|
||
| - name: Run Python script | ||
| shell: bash -l {0} | ||
| run: | | ||
| conda activate test-env | ||
| python3 script.py |
There was a problem hiding this comment.
Fix job name syntax and approve Conda test job.
The test-conda job is well-structured and provides a good test for the package installation and basic functionality using Conda. However, there's a syntax issue with the job name.
To fix the syntax issue, modify the job definition as follows:
test-conda:
name: Test Conda Installation
runs-on: ubuntu-latest
# ... rest of the job definitionThis change adds the name key, which is expected in the job section.
The overall structure of the Conda test job is good:
- It correctly sets up a Miniconda environment.
- It installs the package from conda-forge.
- The test script checks basic functionality by fetching the latest block.
This addition will help ensure the package works correctly when installed via Conda.
🧰 Tools
🪛 actionlint
25-25: unexpected key "test-conda" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
.github/workflows/release.yml
Outdated
| test-pip: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| platform: [linux/amd64] | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: 3.11 | ||
|
|
||
| - name: Install injective-py with pip | ||
| run: | | ||
| pip3 install injective-py | ||
|
|
||
| - name: Write a Python script | ||
| run: | | ||
| echo " | ||
| import asyncio | ||
|
|
||
| from pyinjective.async_client import AsyncClient | ||
| from pyinjective.core.network import Network | ||
|
|
||
|
|
||
| async def main() -> None: | ||
| network = Network.testnet() | ||
| client = AsyncClient(network) | ||
| latest = await client.fetch_latest_block() | ||
| print(latest) | ||
|
|
||
| asyncio.get_event_loop().run_until_complete(main()) | ||
|
|
||
| " > script.py | ||
|
|
||
| - name: Run Python script | ||
| run: | | ||
| python3 script.py |
There was a problem hiding this comment.
Fix job name syntax and approve pip test job.
The test-pip job is well-structured and provides a good test for the package installation and basic functionality using pip. However, there's a syntax issue with the job name, similar to the test-conda job.
To fix the syntax issue, modify the job definition as follows:
test-pip:
name: Test Pip Installation
runs-on: ubuntu-latest
# ... rest of the job definitionThis change adds the name key, which is expected in the job section.
The overall structure of the pip test job is good:
- It correctly sets up Python.
- It installs the package using pip.
- The test script checks basic functionality by fetching the latest block, consistent with the Conda test.
This addition will help ensure the package works correctly when installed via pip.
🧰 Tools
🪛 actionlint
76-76: unexpected key "test-pip" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
PavelInjective
force-pushed
the
add_conda_pip_install_workflow
branch
from
0dfbd9f to
45efb35
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
.github/workflows/release.yml (1)
Line range hint
1-116: Consider adding job dependencies for proper sequencing.The addition of test jobs for both Conda and pip installations is a great improvement to the workflow. However, the current structure allows all jobs to run in parallel. To ensure that the package is first published before running the installation tests, consider adding job dependencies.
You can modify the workflow to make the test jobs depend on the successful completion of the deploy job. This can be achieved by adding a
needskey to both test jobs:test-conda: needs: deploy name: Test Conda Installation # ... rest of the job definition test-pip: needs: deploy name: Test Pip Installation # ... rest of the job definitionThis change will ensure that the installation tests run only after the package has been successfully published to PyPI, providing a more accurate test of the published package.
🧰 Tools
🪛 actionlint
21-21: shellcheck reported issue in this script: SC2086:info:1:31: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: unexpected key "test-conda" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
76-76: unexpected key "test-pip" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/release.yml (2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/release.yml
25-25: unexpected key "test-conda" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
76-76: unexpected key "test-pip" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
.github/workflows/release.yml
Outdated
| test-conda: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| platform: [linux/amd64] | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up Miniconda | ||
| uses: conda-incubator/setup-miniconda@v2 | ||
| with: | ||
| auto-activate-base: false | ||
| python-version: 3.11 | ||
| miniconda-version: "latest" | ||
| architecture: x86_64 | ||
| activate-environment: test-env | ||
|
|
||
| - name: Create conda environment and install injective-py | ||
| shell: bash -l {0} | ||
| run: | | ||
| conda create -n test-env -y | ||
| conda activate test-env | ||
| conda install -c conda-forge injective-py -y | ||
|
|
||
| - name: Write a Python script | ||
| run: | | ||
| echo " | ||
| import asyncio | ||
|
|
||
| from pyinjective.async_client import AsyncClient | ||
| from pyinjective.core.network import Network | ||
|
|
||
|
|
||
| async def main() -> None: | ||
| network = Network.testnet() | ||
| client = AsyncClient(network) | ||
| latest = await client.fetch_latest_block() | ||
| print(latest) | ||
|
|
||
| asyncio.get_event_loop().run_until_complete(main()) | ||
|
|
||
| " > script.py | ||
|
|
||
| - name: Run Python script | ||
| shell: bash -l {0} | ||
| run: | | ||
| conda activate test-env | ||
| python3 script.py |
There was a problem hiding this comment.
Fix job name syntax and approve Conda test job.
The test-conda job is well-structured and provides a good test for the package installation and basic functionality using Conda. However, there's a syntax issue with the job name that needs to be addressed.
To fix the syntax issue, modify the job definition as follows:
test-conda:
name: Test Conda Installation
runs-on: ubuntu-latest
# ... rest of the job definitionThis change adds the name key, which is expected in the job section.
The overall structure of the Conda test job is good:
- It correctly sets up a Miniconda environment.
- It installs the package from conda-forge.
- The test script checks basic functionality by fetching the latest block.
This addition will help ensure the package works correctly when installed via Conda.
🧰 Tools
🪛 actionlint
25-25: unexpected key "test-conda" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
.github/workflows/release.yml
Outdated
| test-pip: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| platform: [linux/amd64] | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: 3.11 | ||
|
|
||
| - name: Install injective-py with pip | ||
| run: | | ||
| pip3 install injective-py | ||
|
|
||
| - name: Write a Python script | ||
| run: | | ||
| echo " | ||
| import asyncio | ||
|
|
||
| from pyinjective.async_client import AsyncClient | ||
| from pyinjective.core.network import Network | ||
|
|
||
|
|
||
| async def main() -> None: | ||
| network = Network.testnet() | ||
| client = AsyncClient(network) | ||
| latest = await client.fetch_latest_block() | ||
| print(latest) | ||
|
|
||
| asyncio.get_event_loop().run_until_complete(main()) | ||
|
|
||
| " > script.py | ||
|
|
||
| - name: Run Python script | ||
| run: | | ||
| python3 script.py |
There was a problem hiding this comment.
Fix job name syntax and approve pip test job.
The test-pip job is well-structured and provides a good test for the package installation and basic functionality using pip. However, there's a syntax issue with the job name that needs to be addressed.
To fix the syntax issue, modify the job definition as follows:
test-pip:
name: Test Pip Installation
runs-on: ubuntu-latest
# ... rest of the job definitionThis change adds the name key, which is expected in the job section.
The overall structure of the pip test job is good:
- It correctly sets up Python.
- It installs the package using pip.
- The test script checks basic functionality by fetching the latest block, consistent with the Conda test.
This addition will help ensure the package works correctly when installed via pip.
🧰 Tools
🪛 actionlint
76-76: unexpected key "test-pip" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
|
It would be advisable for someone in this project to request being a maintainer for the |
There was a problem hiding this comment.
We should not mix the release workflow with a test workflow.
Also I think the test should be run on PRs, not when something is merged in master (at that time, if there is an issue it is already late).
The validation should be run on the still not published package version. The package should be generated locally and should be installed by PIP from the local source for the test workflow.
One extra question: do we really need to create a new test script putting the code in the validation workflow? Why don't we just run any of the scripts already in the project?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
.github/workflows/test-pip.yml (1)
13-20: Update the checkout action to the latest version.The workflow is using
actions/checkout@v3. Consider updating to the latest version (v4) for potential improvements and bug fixes.Apply this change:
- uses: actions/checkout@v3 + uses: actions/checkout@v4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/test-pip.yml (1 hunks)
🔇 Additional comments (2)
.github/workflows/test-pip.yml (2)
1-4: LGTM: Workflow name and trigger are well-defined.The workflow name accurately describes its purpose, and triggering on pull requests is appropriate for testing changes before merging.
22-29: Clarify repository cloning and package installation steps.
The workflow clones a separate
sdk-pythonrepository. Is this necessary? If the package being tested is in the current repository, this step might be redundant.The installation step doesn't specify any dependencies. Consider using a
requirements.txtfile or specifying dependencies insetup.pyto ensure all necessary packages are installed.To check if the
sdk-pythonrepository is necessary, let's verify the content of the current repository:This will help determine if the separate repository clone is necessary.
| jobs: | ||
| test-pip: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| platform: [linux/amd64] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider expanding the matrix strategy.
The job configuration is good, but the matrix strategy currently only includes one platform (linux/amd64). Consider expanding this to test on multiple operating systems and Python versions for more comprehensive coverage.
You could modify the matrix strategy as follows:
strategy:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
python-version: [3.8, 3.9, '3.10', '3.11']Then, update the runs-on to use ${{ matrix.os }} and the Python setup step to use ${{ matrix.python-version }}.
| - name: Run Python script | ||
| run: | | ||
| python3 examples/chain_client/tendermint/query/3_GetLatestBlock.py |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the script execution step.
The current script execution step has a few potential improvements:
- The script path is hardcoded, which could break if the repository structure changes.
- There's no error handling or output capturing.
Consider the following enhancements:
- name: Run Python script
run: |
SCRIPT_PATH="examples/chain_client/tendermint/query/3_GetLatestBlock.py"
if [ -f "$SCRIPT_PATH" ]; then
python3 "$SCRIPT_PATH"
else
echo "Error: Script not found at $SCRIPT_PATH"
exit 1
fi
continue-on-error: true
- name: Check script execution
run: |
if [ ${{ steps.run_script.outcome }} == 'failure' ]; then
echo "Script execution failed. Please check the logs for details."
exit 1
fiThis modification:
- Uses a variable for the script path, making it easier to update.
- Checks if the script exists before attempting to run it.
- Uses
continue-on-errorto ensure the workflow doesn't stop immediately on script failure. - Adds a separate step to check the outcome and provide appropriate feedback.
Summary by CodeRabbit
test-condaandtest-pip) to validate package installation and functionality in both Conda and pip environments.