[FLINK-24933][python] Support ML Python API to implement FLIP-173 and FLP-174 #36

HuangXingBo · 2021-11-17T08:06:13Z

What is the purpose of the change

This PR adds the ML python API to implement FLIP-173 and FLIP-174.

Brief change log

This PR adds classes in the package of api/core and param.

Verifying this change

test_pipeline.py and test_stage.py.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes)

lindong28

Thanks for the PR. Looks pretty good! Left some minor comments below.

Could you update the PR description to explain the scope of this PR? It may be useful to explain that the goal is to implement FLIP-173 and FLIP-174 in python. And we don't support interaction between Java and Python yet.

lindong28 · 2021-11-18T08:14:25Z

flink-ml-python/README.md

@@ -0,0 +1,17 @@
+Flink ML is a library which provides machine learning (ML) APIs and libraries that simplify the building of machine learning pipelines. It provides a set of standard ML APIs for MLlib developers to implement ML algorithms, as well as libraries of ML algorithms that can be used to build ML pipelines for both training and inference jobs.


Do we need to explain flink-ml-python specific instructions in this READMe, similar to flink-python/README.md?

Yes. I will adjust these descriptions.

lindong28 · 2021-11-18T08:17:45Z

flink-ml-python/apache_flink_ml/mllib/__init__.py

@@ -0,0 +1,17 @@
+################################################################################


Should this file be under path flink-ml-python/apache_flink_ml/ml/lib to be consistent with e.g. flink-ml-python/apache_flink_ml/ml/api?

lindong28 · 2021-11-18T08:28:48Z

flink-ml-python/setup.py

+                'apache_flink_ml.ml.param',
+                'apache_flink_ml.ml.util',
+                'apache_flink_ml.examples',
+                'apache_flink_ml.mllib']


It seems that apache_flink_ml.ml.lib would be more consistent with other package names?

lindong28 · 2021-11-18T08:44:53Z

flink-ml-python/apache_flink_ml/ml/api/core.py

+        model_stages = []
+        last_inputs = inputs
+        for i, stage in enumerate(self._stages):
+            if not isinstance(stage, AlgoOperator):


Do we need this check?

If there are no other non-AlgoOperator inherited from Stage class, we can remove this check.

Hmm.. I think we allow Estimator as a member stage of pipeline. And Estimator does not inherit AlgoOperator. Thus this check needs to be removed?

lindong28 · 2021-11-18T08:47:49Z

flink-ml-python/apache_flink_ml/ml/param/param.py

+    Interface for classes that take parameters. It provides APIs to set and get parameters.
+    """
+
+    def set(self, param: 'Param[V]', value: V) -> 'T':


The logic of set(...) in Java was recently updated such that it throws exception if the param is not already defined on this class. Could we do the same here?

lindong28 · 2021-11-18T08:58:53Z

flink-ml-python/apache_flink_ml/ml/tests/test_stage.py

+        return self._param_map
+
+    def _init_param(self):
+        self._param_map[BOOLEAN_PARAM] = BOOLEAN_PARAM.default_value


Is there any way to initialize the param_map automatically without requiring users to manually setup this map? This can be done in Java using reflection (see ParamUtils.initializeMapWithDefaultValues(...)).

Technically, this way can be achieved. But the reason I did not achieve this is that this usage does not seem to be consistent with the idiom of Python users. From my perspective, Python users generally do not use static members(class fields in Python) like this type to define member variables.

Sounds good. I don't know as much about Python idiom. Please feel free to keep this pattern if it fits Python idiom better.

HuangXingBo · 2021-11-19T06:22:40Z

@lindong28 Thanks a lot for the review. I have addressed the comments at the latest commit.

lindong28

@HuangXingBo Thanks for the PR ! LGTM overall. Can you update the commit message as appropriate?

lindong28 · 2021-11-24T06:24:22Z

flink-ml-python/apache_flink_ml/version.py

+The version will be consistent with the flink ml version and follow the PEP440.
+.. seealso:: https://www.python.org/dev/peps/pep-0440
+"""
+__version__ = "0.1.dev0"


Could we rename the directory name from apache_flink_ml to pyflink? This could make the flink-ml directory structure more consistent with the core Flink repo. And we can let the python packages of flink-ml and the core flink could be installed in the same directory under site-packages.

Make sense.

lindong28 · 2021-11-24T06:57:22Z

flink-ml-python/README.md

+
+The sdist package of apache-flink-ml will be found under ./flink-ml-python/dist/. It could be used for installation, such as:
+```bash
+python -m pip install dist/*.tar.gz


Could you update README to specify how to run test cases (similar to the existing README in flink/flink-python/README.md)?

And we probably also need to compile the python package and run all python unit tests in the Github CI. This could be specified in .github/workflows/java8-build.yml. Could you help do this?

README.md is for users, not for developers, so I don't think it is appropriate to add the content of how to test into README.md. Regarding the part of adding python CI, I intend to do it in a separate PR, thanks for the reminder.

Sounds good.

… FLP-174

… FLP-174 This closes apache#36.

lindong28 reviewed Nov 18, 2021

View reviewed changes

HuangXingBo changed the title ~~[FLINK-24933][ML] Support ML Python API~~ [FLINK-24933][ML] Support ML Python API to implement FLIP-173 and FLP-174 Nov 19, 2021

HuangXingBo changed the title ~~[FLINK-24933][ML] Support ML Python API to implement FLIP-173 and FLP-174~~ [FLINK-24933][python] Support ML Python API to implement FLIP-173 and FLP-174 Nov 24, 2021

lindong28 reviewed Nov 24, 2021

View reviewed changes

HuangXingBo force-pushed the FLINK-24933 branch from b0cd9c6 to 85f3ef2 Compare November 24, 2021 06:56

lindong28 reviewed Nov 24, 2021

View reviewed changes

HuangXingBo force-pushed the FLINK-24933 branch 2 times, most recently from 06c83fc to 15cf2c9 Compare November 24, 2021 09:55

HuangXingBo added 4 commits November 29, 2021 10:02

[FLINK-24933][python] Support ML Python API to implement FLIP-173 and…

a777b1c

… FLP-174

[FLINK-24933][python] fix comments-1

027db72

[[FLINK-24933][python] fix comments-2-Rename the package to pyflink

888e8e3

rename module names

238a667

HuangXingBo force-pushed the FLINK-24933 branch from 15cf2c9 to 238a667 Compare November 29, 2021 03:48

dianfu approved these changes Dec 1, 2021

View reviewed changes

HuangXingBo closed this in 835d8d0 Dec 1, 2021

weibozhao pushed a commit to weibozhao/flink-ml that referenced this pull request Dec 8, 2021

[FLINK-24933][python] Support ML Python API to implement FLIP-173 and…

a96ae52

… FLP-174 This closes apache#36.

		@@ -0,0 +1,17 @@
		Flink ML is a library which provides machine learning (ML) APIs and libraries that simplify the building of machine learning pipelines. It provides a set of standard ML APIs for MLlib developers to implement ML algorithms, as well as libraries of ML algorithms that can be used to build ML pipelines for both training and inference jobs.

		@@ -0,0 +1,17 @@
		################################################################################

[FLINK-24933][python] Support ML Python API to implement FLIP-173 and FLP-174 #36

[FLINK-24933][python] Support ML Python API to implement FLIP-173 and FLP-174 #36

Uh oh!

Conversation

HuangXingBo commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Uh oh!

lindong28 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuangXingBo commented Nov 19, 2021

Uh oh!

lindong28 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HuangXingBo commented Nov 17, 2021 •

edited

Loading