[#2518] feat(PyClient): Integrate pytest with Gradle to run python test with CICD #2753
Conversation
|
hi @shaofengshi @zhaoyongjie @coolderli Please help me review this PR, Thanks! |
|
Hi Xun, I have two comments:
For other parts, it looks good to me |
|
hi @shaofengshi
Yes, Because Python Gradle plugin is very few, I didn't found better than to
I didn't found a Python Gradle plugin would to support venv. I think we could use it first. |
OK, we can go with this first, and refine later. |
| } | ||
|
|
||
| compileJava { | ||
| dependsOn(pipInstall) |
There was a problem hiding this comment.
should the compile and spotless depend on pip install? Python doesn't need to compile and can't verify the correctness of the program. If the user only updates the Java code, they also need to download the dependencies of the Python module. How about just making the test depend on pip install?
There was a problem hiding this comment.
Yes, Currently use Gradle to comple Python module have this limit (need add compileJava).
I Create an issue #2770 to track this probleam.
There was a problem hiding this comment.
hi @coolderli I fixed this problem in this PR. :-)
|
Is this python environment removable, where is it installed? |
Automatic download |
|
@shaofengshi @zhaoyongjie @coolderli Please help me review this PR, Thanks. |
clients/client-python/setup.py
Outdated
| @@ -9,7 +9,7 @@ | |||
| setup( | |||
| name="gravitino", | |||
| description="project description TBD", | |||
There was a problem hiding this comment.
We can put some description, like "Python lib/client for Gravitino"
| - api/** | ||
| - bin/** | ||
| - catalogs/** | ||
| - clients/client-java/** |
There was a problem hiding this comment.
Are there too many file directories involved here? If the clients/client-java, clients/client-java-runtime changes, I think it doesn't need to run the python-integration-test CICD. What do you think?
There was a problem hiding this comment.
Thank you for your suggestion.
|
|
||
| - name: Package Gravitino | ||
| run: | | ||
| ./gradlew build -x test -PjdkVersion=${{ matrix.java-version }} |
There was a problem hiding this comment.
Can we just build the clients:client-python here? The whole build step has already been covered by build.yml.
There was a problem hiding this comment.
Thank you for your suggestion. I modified PR.
|
@xunliu Hi, I left two comments. Overall, LGTM. Thanks. |
…e#2684) ### What changes were proposed in this pull request? - use Gradle to manage Python build. - add Github CI - `./gradlew clean clients:client-python:test` ### Why are the changes needed? Fix: apache#2518 ### Does this PR introduce _any_ user-facing change? - no ### How was this patch tested? - `./gradlew clean clients:client-python:test`
|
LGTM |
What changes were proposed in this pull request?
gravitino/.gradle/pythondirectory3.8,3.9,3.10,and3.11test ITs in Github Actiongradlew clean&gradlew build&gradlew :client:client-python:test -PpythonVersion=3.8Why are the changes needed?
Use Gradle manager and development Python client module.
Fix: #2518
Does this PR introduce any user-facing change?
pythonVersionGradle parameter setting Python version to compile and test Python client,gradlew :client:client-python:test -PpythonVersion=3.8skipPythonITsGradle parameter to control if skip Python test cases.How was this patch tested?
CI Passed.