-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[FLINK-30277][python]Allow PYTHONPATH of Python Worker configurable #21770
Conversation
d10e432
to
c596d8c
Compare
649e1e4
to
1431e69
Compare
flink-python/src/main/java/org/apache/flink/python/PythonOptions.java
Outdated
Show resolved
Hide resolved
flink-python/src/main/java/org/apache/flink/python/env/AbstractPythonEnvironmentManager.java
Outdated
Show resolved
Hide resolved
@HuangXingBo |
dc50cfb
to
b07163a
Compare
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.
@Samrat002 Thanks a lot for the feature. I have left some comments.
if (pythonEnv.pythonPath != null) { | ||
String defaultPythonPath = env.get("PYTHONPATH"); | ||
String defaultPythonPath = | ||
config.getOptional(PYTHON_PATH).orElse(env.get("PYTHON_PATH")); |
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.
config.getOptional(PYTHON_PATH).orElse(env.get("PYTHON_PATH")); | |
config.getOptional(PYTHON_PATH).orElse(env.get("PYTHONPATH")); |
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.
Will the PYTHON_PATH
configuration work on both the client and the cluster side?
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.
And I prefer these logic is in preparePythonEnvironment
rather than adding another param ReadableConfig
to startPythonProcess
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.
Will the PYTHON_PATH configuration work on both the client and the cluster side?
Yes this is for both client side and cluster side.
@@ -6,7 +6,7 @@ info: | |||
license: | |||
name: Apache 2.0 | |||
url: https://www.apache.org/licenses/LICENSE-2.0.html | |||
version: v1/1.17-SNAPSHOT | |||
version: v1/1.18-SNAPSHOT |
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.
Modifications not related to this feature?
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.
This is related to this feature change only.
This doc update is generated after running the below command
./mvnw package -Dgenerate-rest-docs -pl flink-docs -am -nsu -DskipTests
68a18b1
to
29704c4
Compare
29704c4
to
db6e01e
Compare
Thank you for initially reviewing the changes. @HuangXingBo |
7684db4
to
29821f3
Compare
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.
@Samrat002 Thanks a lot for the update. I have left some comments.
docs/layouts/shortcodes/generated/execution_config_configuration.html
Outdated
Show resolved
Hide resolved
flink-python/src/main/java/org/apache/flink/python/PythonOptions.java
Outdated
Show resolved
Hide resolved
flink-python/src/main/java/org/apache/flink/python/env/PythonDependencyInfo.java
Outdated
Show resolved
Hide resolved
flink-python/src/main/java/org/apache/flink/python/env/PythonDependencyInfo.java
Outdated
Show resolved
Hide resolved
flink-python/src/main/java/org/apache/flink/python/env/AbstractPythonEnvironmentManager.java
Outdated
Show resolved
Hide resolved
flink-python/src/main/java/org/apache/flink/python/env/PythonDependencyInfo.java
Outdated
Show resolved
Hide resolved
flink-python/src/main/java/org/apache/flink/python/PythonOptions.java
Outdated
Show resolved
Hide resolved
217826e
to
5722222
Compare
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.
@Samrat002 Thanks a lot for the update. Only a comment left which I will update during merging.
flink-python/src/main/java/org/apache/flink/client/python/PythonEnvUtils.java
Outdated
Show resolved
Hide resolved
Co-authored-by: HuangXingBo <hxbks2ks@gmail.com>
13b09c4
to
b739eef
Compare
What is the purpose of the change
Currently, below are the ways Python Worker gets the Python Flink Dependencies.
-pyfs
and--pyarch
which is localised intoPYTHONPATH
of Python Worker.-requirement.txt
which gets installed on Worker Node and added intoPYTHONPATH
of Python Worker.This change allow
PYTHONPATH
of Python Worker configurable where Admin/Service provider can install the required python Flink dependencies on a custom path (/usr/lib/pyflink/lib/python3.7/site-packages
) on all Worker Nodes and then set the path in the client machine configurationflink-conf.yaml
. This way it works without any configurations from the Application Users and also without affecting any other components dependent on System Python Path.Brief change log
add an option to add in flink conf to pick configurable python path in worker and client.
Verifying this change
In
flink-conf.yaml
add the following configs:python.client.executable: python3
python.executable: python3
python.pythonpath: /tmp/PYTHONPATH/lib64/python3.7/site-packages/:/tmp/PYTHONPATH/lib/python3.7/site-packages/
Install PyFlink libraries on Client and Worker nodes
Install python in all worker nodes
Run a job
LOG
jobmanager log
taskmanager log
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no) noDocumentation