Add CI for Python client

[DaniilRoman]

#1058

[flyrain]

LGTM overall. Will appreciate a python person to take a look. cc @HonahX

[MonkeyCanCode]

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.

[HonahX]

+1 I think we should fix the poetry version here to be the one in regtest/requirements.txt

[MonkeyCanCode]

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.

[DaniilRoman]

I must admit, I took it from regtests/client/python/.github/workflows/python.yml assuming that it was verified before

There are quite a few linter complaints, that's true

But at the same time, we can start with smth small like this command

[MonkeyCanCode]

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

[HonahX]

But do we really needed to run this as this is already getting done via

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.

[MonkeyCanCode]

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.

[DaniilRoman]

It passes locally without specifing anything specific for Spark 🤔

[MonkeyCanCode]

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).

[MonkeyCanCode]

This is the SCRIPT_DIR I was referring to:

(polaris-venv) ➜  python git:(main) ✗ poetry run pytest
The "poetry.dev-dependencies" section is deprecated and will be removed in a future version. Use "poetry.group.dev.dependencies" instead.
========================================================================== test session starts ===========================================================================
platform darwin -- Python 3.13.0, pytest-8.3.5, pluggy-1.5.0
rootdir: /Users/yong/Desktop/GitHome/polaris/regtests/client/python
configfile: pyproject.toml
plugins: anyio-4.8.0
collected 208 items / 1 error

================================================================================= ERRORS =================================================================================
_______________________________________________________________ ERROR collecting test/test_cli_parsing.py ________________________________________________________________
test/test_cli_parsing.py:26: in <module>
    from cli.command import Command
cli/command/__init__.py:22: in <module>
    from cli.constants import Commands, Arguments
cli/constants.py:241: in <module>
    raise Exception("The SCRIPT_DIR environment variable is not set. Please set it to the Polaris's script directory.")
E   Exception: The SCRIPT_DIR environment variable is not set. Please set it to the Polaris's script directory.

You can get around it with following:

 SCRIPT_DIR="" poetry run pytest

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 regtests this will include a lot more testings (this will fail as it will need polaris server to be up running and not just mock tests which is the dir you were hitting earlier). Then here is an example script path where we are invoking spark: ./t_pyspark/src/iceberg_spark.py

[DaniilRoman]

I've modified this step by adding SCRIPT_DIR

      - name: Test with pytest
        working-directory: regtests/client/python
        run: |
          export SCRIPT_DIR="non-existing-mock-directory"
          poetry run pytest

It works locally

But I'm not sure about regtests because we already have this step in CI for every PR and I think it should be enough, shouldn't it? 🤔

[DaniilRoman]

As it was nicely noticed in the related PR here #1102 (comment)
This line will cause failure since poetry.lock is in .gitignore

I'd duplicate my question here as well
If we had poetry.lock we could even use this config cache: 'poetry' of actions/setup-python@v5 GitHub action (here's this docs section).

Without having poetry.lock we can only cache based on pyptoject.toml which doesn't cover transitive dependencies.
I've also thought about generating poetry.lock on CI but for this we need to have poetry first and when we install poetry, it installs dependencies as well - so, it wouldn't work

What about removing poetry.lock from .gitignore and creating it and managing for the future changes? 🤔
Or is it good enough to create a hash key based on myproject.toml instead?

[MonkeyCanCode]

Add comment for this in the other PR.

[MonkeyCanCode]

@eric-maynard see above

[github-actions]

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sfc-gh-jojiang]

Suggested change

	push:
	branches: [ "main" ]
	paths:
	- 'client/python/**'
	push:
	branches: [ "main" ]

I think it will be good to always trigger the CI when there is a new commit in main. This allows us to check the correctness periodically to ensure there is no changes in other folder that unexpectedly break the CLI.

Personally, I am in favor of removing the paths parameter in pull_request too to make the CI config aligned with other workflow:

on:
  push:
    branches: [ "main" ]
  pull_request:
    branches: [ "main" ]

[HonahX]

Hi @DaniilRoman, thanks for the PR! If you don’t have time to continue, would you mind if I take it over? I think this would be a great addition for Polaris 1.0.

[DaniilRoman]

Hi @DaniilRoman, thanks for the PR! If you don’t have time to continue, would you mind if I take it over? I think this would be a great addition for Polaris 1.0.

Hi @HonahX 👋
I'd appreciate it if anyone would contribute to this solution 🙏