[FLINK-12881][ml] Add more functionalities for ML Params and ParamInfo class #8776

xuyang1706 · 2019-06-18T08:08:35Z

What is the purpose of the change

Add more functionalities for ML Params and ParamInfo class

Brief change log

change Params$get to support aliases

support aliases
if the Params contains the specific parameter and alias, but has more than one value or
if the Params doesn't contains the specific parameter, while the ParamInfo is optional but has no default value, it will throw exception
when isOptional is true, contain(ParamInfo) is true, it will return the value found in Params, whether the value is null or not. when isOptional is true, contain(ParamInfo) is false: hasDefaultValue is true, it will return defaultValue. hasDefaultValue is false, it will throw exception. developer should use contain to check that Params has ParamInfo or not.

add size, clear, isEmpty, contains, fromJson in Params
fix ParamInfo, PipelineStage to adapt new Params

assert null in alias
use Params$loadJson in PipelineStage

add test cases about aliases

Verifying this change

This change added tests and can be verified as follows:

(example:)

add put null in Params

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

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (javaDocs)

flinkbot · 2019-06-18T08:11:45Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Review Progress

✅ 1. The [description] looks good.
- Approved by @shaoxuan-wang [committer]
✅ 2. There is [consensus] that the contribution should go into to Flink.
- Approved by @shaoxuan-wang [committer]
❓ 3. Needs [attention] from.
✅ 4. The change fits into the overall [architecture].
- Approved by @shaoxuan-wang [committer]
✅ 5. Overall code [quality] is good.
- Approved by @shaoxuan-wang [committer]

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

ex00 · 2019-06-18T15:46:46Z

flink-ml-parent/flink-ml-api/src/main/java/org/apache/flink/ml/api/misc/param/Params.java

+		for (String nameOrAlias : getParamNameAndAlias(info)) {
+			if (params.containsKey(nameOrAlias)) {
+				if (usedParamName != null) {
+					throw new IllegalArgumentException(String.format("Duplicate parameters of %s and %s",


Are you sure about the usefulness of the information in the error message? I suppose usedParamName and nameOrAlias are same here.

usedParamName is the first found in the list returned by getParamNameAndAlias(info), so when the map contain the param name or the alias more than one, it will throw exception.

oh, thanks for the explanation.

Could you please add test case for check it?

added to o.a.f.ml.api.misc.ParamsTest$testGetAliasParam

ghost · 2019-06-19T08:16:42Z

flink-ml-parent/flink-ml-api/src/test/java/org/apache/flink/ml/api/misc/ParamsTest.java

+		 *
+		 * example:
+		 *
+		 * in {@link HasPredDetailColName}, it should be optional, and has not default value.


I suppose we should not link from flink-ml-api model to a param defined in flink-ml-lib. flink-ml-api should be independent. Please check.

xuyang1706 · 2019-06-19T13:23:11Z

@becketqin @walterddr @ex00 @c4emmmm @zentol Thanks for all your comments. It seems all comments have been addressed, please let me know if there are more pending questions.

cc @shaoxuan-wang

becketqin

@xuyang1706 Thanks for the patch. It LGTM overall. I gave a pass and left some comment. Can you take a look?

becketqin · 2019-06-24T01:22:20Z

flink-ml-parent/flink-ml-api/src/main/java/org/apache/flink/ml/api/misc/param/ParamInfo.java


 /**
 * Definition of a parameter, including name, type, default value, validator and so on.
 *
 * <p>This class is provided to unify the interaction with parameters.
 *
+ * <p>when isOptional is true, contain(ParamInfo) is true, it will return the value found in Params,


This class is only about definition. It might be better to keep the semantic of contains() and get() in there own method java doc. How about just mention the following here? Also, should we throw exception if a default value is specified for a non-optional parameter?

/** * Definition of a parameter, including name, type, default value, validator and so on. * * <p>A parameter can either be optional or non-optional. * <ul> * <li> * A non-optional parameter should not have a default value. Instead, its value must be provided by the users. * </li> * <li> * An optional parameter may or may not have a default value. * </li> * </ul> * * <p>Please see {@link Params#get(ParamInfo)} and {@link Params#contains(ParamInfo)} for more details about the behavior. * * <p>A parameter may have aliases in addition to the parameter name for convenience and compatibility purposes. One * should not set values for both parameter name and an alias. One and only one value should be set either under * the parameter name or one of the alias. * * @param <V> the type of the param value */

Thanks, your description is more clear. The default value in the non-optional parameter will never be used, I think it is not necessary to check this by extra codes.

becketqin · 2019-06-24T01:47:32Z

flink-ml-parent/flink-ml-api/src/main/java/org/apache/flink/ml/api/misc/param/Params.java

+	 */
+	public boolean isEmpty() {
+		return params.isEmpty();
+	}

 	/**
 	 * Returns the value of the specific parameter, or default value defined in the {@code info} if


I moved some of the java doc from ParamInfo to here. See if it is clearer.

/** * Returns the value of the specific parameter, or default value defined in the {@code info} if * this Params doesn't have a value set for the parameter. An exception will be thrown in the * following cases because no value could be found for the specified parameter. * <ul> * <li> * Non-optional parameter: no value is defined in this params for a non-optional parameter. * </li> * <li> * Optional parameter: no value is defined in this params and no default value is defined. * </li> * </ul> * * @param info the info of the specific parameter, usually with default value * @param <V> the type of the specific parameter * @return the value of the specific parameter, or default value defined in the {@code info} if * this Params doesn't contain the parameter * @throws IllegalArgumentException if no value can be found for specified parameter */

becketqin · 2019-06-24T01:54:45Z

flink-ml-parent/flink-ml-api/src/main/java/org/apache/flink/ml/api/misc/param/Params.java

-		Params newParams = new Params();
-		newParams.paramMap.putAll(this.paramMap);
-		return newParams;
+	public <V> boolean contains(ParamInfo<V> paramInfo) {


The argument name is info everywhere else. Can we just use info instead of paramInfo here?

Thanks for your advice, info is better.

becketqin · 2019-06-24T02:02:33Z

flink-ml-parent/flink-ml-api/src/main/java/org/apache/flink/ml/api/misc/param/Params.java

 	}

 	/**
-	 * Creates and returns a deep clone of this Params.
+	 * Returns <tt>true</tt> if this params has the specified paramInfo.


I moved some of the java doc from ParamInfo here. See if it is clearer.

/** * Check whether a value is set for the given {@code paramInfo}. A value is considered set in * the following cases: * <ul> * <li> * The given parameter has a value set in this Params, or * </li> * <li> * The given parameter is optional and has a default value defined in the {@code paramInfo} * </li> * </ul> * * If this method returns false, {@link #get(ParamInfo)} will throw an exception. * * @return <tt>true</tt> if this params has a value set for the specified {@code paramInfo}, false otherwise. */

@becketqin , refer your feedback, I change the doc. Could you take a look?

becketqin · 2019-06-24T02:03:55Z

flink-ml-parent/flink-ml-api/src/main/java/org/apache/flink/ml/api/misc/param/Params.java

-		newParams.paramMap.putAll(this.paramMap);
-		return newParams;
+	public <V> boolean contains(ParamInfo<V> paramInfo) {
+		return params.containsKey(paramInfo.getName()) ||


This behavior is different from the java doc. The java doc in ParamInfo said the following:

"when isOptional is true, contain(ParamInfo) is true, it will return the value found in Params, ..."

Thanks, changed.

…o class 1. change Params$get to support aliases * support aliases * if the Params contains the specific parameter and alias, but has more than one value, it will be throw exception * if the Params doesn't contains the specific parameter, while the ParamInfo is optional but has no default value, it will throw exception * when isOptional is true, contain(ParamInfo) is true, it will return the value found in Params, whether the value is null or not. when isOptional is true, contain(ParamInfo) is false: hasDefaultValue is true, it will return defaultValue. hasDefaultValue is false, it will throw exception. developer should use contain to check that Params has ParamInfo or not. 2. add size, clear, isEmpty, contains, fromJson in Params 3. fix ParamInfo, PipelineStage to adapt new Params * assert null in alias * use Params$loadJson in PipelineStage 4. add test cases about aliases

becketqin · 2019-06-25T01:08:10Z

@xuyang1706 Thanks for the update. The latest patch LGTM.
@shaoxuan-wang Do you want to take another look?

…o class This closes apache#8776

shaoxuan-wang · 2019-06-25T10:19:28Z

Thanks @xuyang1706 for the contribution, and @ex00 @becketqin @c4emmmm for the reviews.
The changes look good to me. I will merge it once the CI passes on my repo.

@flinkbot approve all

ex00

Thansk @xuyang1706
LGTM

rmetzger added the review=description? label Jun 18, 2019

xuyang1706 force-pushed the params-api branch from 70582bf to 6fe3582 Compare June 18, 2019 08:28

xuyang1706 mentioned this pull request Jun 18, 2019

[FLINK-12744][ml] add shared params in ml package #8632

Closed

rmetzger added the component=<none> label Jun 18, 2019

ex00 reviewed Jun 18, 2019

View reviewed changes

xuyang1706 force-pushed the params-api branch from 6fe3582 to ed6c46e Compare June 19, 2019 03:37

ghost reviewed Jun 19, 2019

View reviewed changes

xuyang1706 force-pushed the params-api branch from ed6c46e to f295369 Compare June 19, 2019 13:18

xuyang1706 force-pushed the params-api branch 3 times, most recently from 421472b to 3f941dc Compare June 21, 2019 03:45

rmetzger added component=Library/MachineLearning and removed component=<none> labels Jun 21, 2019

becketqin reviewed Jun 24, 2019

View reviewed changes

xuyang1706 force-pushed the params-api branch from 3f941dc to 1734ef3 Compare June 24, 2019 09:55

shaoxuan-wang self-assigned this Jun 25, 2019

shaoxuan-wang pushed a commit to shaoxuan-wang/flink that referenced this pull request Jun 25, 2019

[FLINK-12881][ml] Add more functionalities for ML Params and ParamInf…

4e95ba7

…o class This closes apache#8776

ex00 approved these changes Jun 25, 2019

View reviewed changes

rmetzger added review=approved ✅ and removed review=description? labels Jun 25, 2019

asfgit closed this in 1660c6b Jun 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-12881][ml] Add more functionalities for ML Params and ParamInfo class #8776

[FLINK-12881][ml] Add more functionalities for ML Params and ParamInfo class #8776

xuyang1706 commented Jun 18, 2019 •

edited

flinkbot commented Jun 18, 2019 •

edited

ex00 Jun 18, 2019

xuyang1706 Jun 19, 2019

ex00 Jun 19, 2019

ex00 Jun 19, 2019

xuyang1706 Jun 19, 2019

ex00 Jun 19, 2019

ghost Jun 19, 2019

xuyang1706 Jun 19, 2019

xuyang1706 commented Jun 19, 2019

becketqin left a comment

becketqin Jun 24, 2019

xuyang1706 Jun 24, 2019

becketqin Jun 24, 2019

xuyang1706 Jun 24, 2019

becketqin Jun 24, 2019

xuyang1706 Jun 24, 2019

becketqin Jun 24, 2019

xuyang1706 Jun 24, 2019

becketqin Jun 24, 2019

xuyang1706 Jun 24, 2019

becketqin commented Jun 25, 2019

shaoxuan-wang commented Jun 25, 2019

ex00 left a comment

[FLINK-12881][ml] Add more functionalities for ML Params and ParamInfo class #8776

[FLINK-12881][ml] Add more functionalities for ML Params and ParamInfo class #8776

Conversation

xuyang1706 commented Jun 18, 2019 • edited

What is the purpose of the change

Brief change log

Verifying this change

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

Documentation

flinkbot commented Jun 18, 2019 • edited

Review Progress

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuyang1706 commented Jun 19, 2019

becketqin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

becketqin commented Jun 25, 2019

shaoxuan-wang commented Jun 25, 2019

ex00 left a comment

Choose a reason for hiding this comment

xuyang1706 commented Jun 18, 2019 •

edited

flinkbot commented Jun 18, 2019 •

edited