-
Notifications
You must be signed in to change notification settings - Fork 199
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
Add CI for Python client #1096
base: main
Are you sure you want to change the base?
Add CI for Python client #1096
Changes from 4 commits
9861bd8
389f5e6
210b285
98b2078
5a5c0a5
c7e8ec5
a4d6351
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
# | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
# | ||
|
||
# This workflow uses actions that are not certified by GitHub. | ||
# They are provided by a third-party and are governed by | ||
# separate terms of service, privacy policy, and support | ||
# documentation. | ||
# This workflow will build a Python project with Poetry and cache/restore any dependencies to improve the workflow execution time | ||
# For more information see: https://docs.github.com/en/actions/use-cases-and-examples/building-and-testing/building-and-testing-python | ||
|
||
name: Python CI with Poetry | ||
|
||
on: | ||
push: | ||
branches: [ "main" ] | ||
paths: | ||
- 'regtests/client/python/**' | ||
pull_request: | ||
branches: [ "main" ] | ||
paths: | ||
- 'regtests/client/python/**' | ||
|
||
jobs: | ||
build: | ||
|
||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: ["3.7", "3.8", "3.9", "3.10", "3.11"] | ||
|
||
steps: | ||
- name: Checkout Polaris project | ||
uses: actions/checkout@v4 | ||
|
||
- name: Set up Python ${{ matrix.python-version }} | ||
uses: actions/setup-python@v5 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
|
||
- name: Cache Poetry | ||
id: cache-poetry | ||
uses: actions/cache@v4 | ||
with: | ||
path: ~/.cache/pypoetry | ||
key: ${{ runner.os }}-poetry-${{ hashFiles('regtests/client/python/poetry.lock') }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As it was nicely noticed in the related PR here #1102 (comment) I'd duplicate my question here as well Without having What about removing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add comment for this in the other PR. |
||
restore-keys: | | ||
${{ runner.os }}-poetry- | ||
|
||
- name: Install Poetry | ||
if: steps.cache-poetry.outputs.cache-hit != 'true' | ||
run: | | ||
curl -sSL https://install.python-poetry.org | python3 - | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may cause issue as this will install poetry 2.x version on python >=3.9 while the repo is still on 1.8.x version. There is a PR from me for upgrading to 2.x version in case if we want to proceed with this: #898 But even with this PR, it is still not a good idea to run latest version as the above command can install newer version of poetry which is not being tested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 I think we should fix the poetry version here to be the one in |
||
export PATH="$HOME/.local/bin:$PATH" | ||
|
||
- name: Install dependencies | ||
working-directory: regtests/client/python | ||
run: poetry install | ||
|
||
- name: Lint with flake8 | ||
working-directory: regtests/client/python | ||
run: | | ||
# stop the build if there are Python syntax errors or undefined names | ||
poetry run flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics | ||
# exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide | ||
poetry run flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this line will have exit-zero which is a free-pass regardless what people check-in (also the existed code doesn't follow the these rules as well). Should we consider fix existed code first then ensure no other warnings/errors are shows? As if we go with current code, this is will still be ignore and it is always pass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
- name: Test with pytest | ||
working-directory: regtests/client/python | ||
run: poetry run pytest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not every test used by the current test cases are on pytest and running this command will throw error (due to one of the recent change for profile setting which we needed script dir to be set). But do we really needed to run this as this is already getting done via https://github.com/apache/polaris/blob/main/.github/workflows/regtest.yml#L55 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it's still worth running the client tests here because they help quickly isolate errors specific to the client. Additionally, the regtest workflow doesn’t cover all supported Python versions, and it's beneficial to test across different versions since users may run the CLI in various environments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. K, in that case, we will need to set a variable for SCRIPT_DIR to be able to run this one. Also, some of the test cases may needed spark as well which is not getting install from poetry as of now. I had a PR to switch from local installed spark to python but that was removed due to too many changes. So we may want to introduce that change again or setup spark as how the current test setup is doing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It passes locally without specifing anything specific for Spark 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will check a bit later tonight for this. It is possible as not all tests are pytest (in this case, the spark related ones). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the SCRIPT_DIR I was referring to:
You can get around it with following:
Also, quick checks shows we do have spark deps but you are not hitting it as you are on the wrong test dir. We should run test on |
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.
According to https://github.com/apache/polaris/blob/main/regtests/client/python/pyproject.toml#L33, the min supported python version is 3.8. Also, with the command later on where we will be installing newest poetry, it will actually needed at least python 3.9 instead.