-
Notifications
You must be signed in to change notification settings - Fork 87
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-22915][FLIP-173] Update Flink ML API to support AlgoOperator with multiple input tables and multiple output tables #4
Conversation
@becketqin Would you have time to review this PR? |
flink-ml-api/src/main/java/org/apache/flink/ml/api/core/Pipeline.java
Outdated
Show resolved
Hide resolved
flink-ml-api/src/main/java/org/apache/flink/ml/api/core/PipelineModel.java
Show resolved
Hide resolved
flink-ml-api/src/main/java/org/apache/flink/ml/api/core/Pipeline.java
Outdated
Show resolved
Hide resolved
2d26a36
to
d4f4491
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.
@zhipeng93 Thanks for the review :)
Can we delay the review/code of the save/load and also related implementation of Pipeline/PipelineModel to a later PR? The code/test for these features are available in https://github.com/lindong28/flink-ml/tree/flink-ml if you want to take a look earlier to address the concern regarding e.g. lazy sink.
@lindong28 Thanks :) |
@zhipeng93 Sure. Can you explain the concern? And do you think these concern needs to be addressed in this PR? |
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.
@lindong28 Thanks for the patch. It looks that CI is not working properly for this repo. We probably need to fix that. I tried to build the project on my local machine after fixing the package path issue, but still failed in flink-ml-examples-streaming
module. It looks we have Scala dependency there which should probably be removed completely. Have you tried build the project locally? Did it pass?
flink-ml-api/src/main/java/org/apache/flink/ml/api/core/Pipeline.java
Outdated
Show resolved
Hide resolved
flink-ml-api/src/main/java/org/apache/flink/ml/api/core/Pipeline.java
Outdated
Show resolved
Hide resolved
flink-ml-api/src/main/java/org/apache/flink/ml/api/core/PipelineModel.java
Outdated
Show resolved
Hide resolved
flink-ml-api/src/main/java/org/apache/flink/ml/api/core/PipelineModel.java
Outdated
Show resolved
Hide resolved
Thanks for the review @becketqin. I did Yes we can remove Scala dependency later. For now let's keep it because the Alink's infra used for KMeans (see https://github.com/zhipeng93/flink-ml/tree/alink) has Scala code. |
8c53d5b
to
76fd1a5
Compare
…ith multiple input tables and multiple output tables
76fd1a5
to
4e44524
Compare
@becketqin After looking into the package path issue, I think the CI is actually working correctly. Here is what I find:
|
Thanks for the patch. LGTM. |
What is the purpose of the change
This PR updates the Flink ML API according to FLIP-173.
Brief change log
This PR mades the following changes:
Verifying this change
This PR focuses on changing the Flink ML API. Some APIs such as save/load needs to be implemented after we complete the design of the parameter interface.
We will have followup PRs to implement all the Flink ML APIs and provide better test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes)Documentation