lindong28 commented on a change in pull request #71: URL: https://github.com/apache/flink-ml/pull/71#discussion_r841074926
########## File path: flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/data/InputDataGeneratorParams.java ########## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.ml.benchmark.data; + +import org.apache.flink.ml.param.LongParam; +import org.apache.flink.ml.param.Param; +import org.apache.flink.ml.param.ParamValidators; +import org.apache.flink.ml.param.StringArrayParam; + +/** Interface for the input data generator params. */ +public interface InputDataGeneratorParams<T> extends DataGeneratorParams<T> { Review comment: Would it be simpler to remove this class and define parameters directly in `InputDataGenerator`? By doing this, all subclasses of `InputDataGenerator`, such as `DenseVectorArrayGenerator` and `DenseVectorGenerator`, would inherit those parameters. Note that we have dedicated parameter classes for algorithms because an algorithm typically have a pair of estimator and model, where estimator and model share some but not call parameters. Data generators do not have have this problem. Similarly, it might be simpler to remove the generator-specific parameter classes such as `DenseVectorArrayGeneratorParams` and `DenseVectorGeneratorParams`. We can still define non-generator-specific parameters in dedicated classes such as `HasArraySize`. ########## File path: flink-ml-benchmark/src/test/java/org/apache/flink/ml/benchmark/BenchmarkTest.java ########## @@ -0,0 +1,185 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.ml.benchmark; + +import org.apache.flink.ml.benchmark.data.InputDataGenerator; +import org.apache.flink.ml.benchmark.data.common.DenseVectorGenerator; +import org.apache.flink.ml.clustering.kmeans.KMeans; +import org.apache.flink.ml.clustering.kmeans.KMeansModel; +import org.apache.flink.ml.param.WithParams; +import org.apache.flink.ml.util.ReadWriteUtils; +import org.apache.flink.streaming.api.environment.StreamExecutionEnvironment; +import org.apache.flink.table.api.bridge.java.StreamTableEnvironment; +import org.apache.flink.test.util.AbstractTestBase; + +import org.apache.commons.io.FileUtils; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.InputStream; +import java.io.PrintStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +import static org.apache.flink.ml.util.ReadWriteUtils.OBJECT_MAPPER; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** Tests benchmarks. */ +@SuppressWarnings("unchecked") +public class BenchmarkTest extends AbstractTestBase { + @Rule public final TemporaryFolder tempFolder = new TemporaryFolder(); + + private static final String exampleConfigFile = "kmeansmodel-benchmark.json"; + private static final String expectedBenchmarkName = "KMeansModel-1"; + + private final ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + private final PrintStream originalOut = System.out; + + @Before + public void before() { + System.setOut(new PrintStream(outputStream)); + } + + @After + public void after() { + System.setOut(originalOut); + outputStream.reset(); + } + + @Test + public void testCreateAndExecute() throws Exception { Review comment: Would it be more intuitive to rename this test as `testRunBenchmark()`? ########## File path: flink-ml-benchmark/src/test/java/org/apache/flink/ml/benchmark/BenchmarkTest.java ########## @@ -0,0 +1,185 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.ml.benchmark; + +import org.apache.flink.ml.benchmark.data.InputDataGenerator; +import org.apache.flink.ml.benchmark.data.common.DenseVectorGenerator; +import org.apache.flink.ml.clustering.kmeans.KMeans; +import org.apache.flink.ml.clustering.kmeans.KMeansModel; +import org.apache.flink.ml.param.WithParams; +import org.apache.flink.ml.util.ReadWriteUtils; +import org.apache.flink.streaming.api.environment.StreamExecutionEnvironment; +import org.apache.flink.table.api.bridge.java.StreamTableEnvironment; +import org.apache.flink.test.util.AbstractTestBase; + +import org.apache.commons.io.FileUtils; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.InputStream; +import java.io.PrintStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +import static org.apache.flink.ml.util.ReadWriteUtils.OBJECT_MAPPER; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** Tests benchmarks. */ +@SuppressWarnings("unchecked") +public class BenchmarkTest extends AbstractTestBase { + @Rule public final TemporaryFolder tempFolder = new TemporaryFolder(); + + private static final String exampleConfigFile = "kmeansmodel-benchmark.json"; + private static final String expectedBenchmarkName = "KMeansModel-1"; + + private final ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + private final PrintStream originalOut = System.out; + + @Before + public void before() { + System.setOut(new PrintStream(outputStream)); + } + + @After + public void after() { + System.setOut(originalOut); + outputStream.reset(); + } + + @Test + public void testCreateAndExecute() throws Exception { + StreamExecutionEnvironment env = StreamExecutionEnvironment.getExecutionEnvironment(); + StreamTableEnvironment tEnv = StreamTableEnvironment.create(env); + + KMeans kMeans = new KMeans().setK(5).setFeaturesCol("test_feature"); + InputDataGenerator<?> inputsGenerator = + new DenseVectorGenerator() + .setColNames("test_feature") + .setNumValues(1000) + .setVectorDim(10); + + BenchmarkResult result = + BenchmarkUtils.runBenchmark(tEnv, "testBenchmarkName", kMeans, inputsGenerator); + + BenchmarkUtils.printResults(result); Review comment: Do you see any existing example where we check the value by printing it to stdout and check the printed stream? I find it pretty uncommon to do this, which suggests that it is not following the best practice. Would it be simpler to check the value of `result` directly, instead of printing its to stdout and checking the printed String? ########## File path: flink-ml-benchmark/src/test/java/org/apache/flink/ml/benchmark/BenchmarkTest.java ########## @@ -0,0 +1,185 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.ml.benchmark; + +import org.apache.flink.ml.benchmark.data.InputDataGenerator; +import org.apache.flink.ml.benchmark.data.common.DenseVectorGenerator; +import org.apache.flink.ml.clustering.kmeans.KMeans; +import org.apache.flink.ml.clustering.kmeans.KMeansModel; +import org.apache.flink.ml.param.WithParams; +import org.apache.flink.ml.util.ReadWriteUtils; +import org.apache.flink.streaming.api.environment.StreamExecutionEnvironment; +import org.apache.flink.table.api.bridge.java.StreamTableEnvironment; +import org.apache.flink.test.util.AbstractTestBase; + +import org.apache.commons.io.FileUtils; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.InputStream; +import java.io.PrintStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +import static org.apache.flink.ml.util.ReadWriteUtils.OBJECT_MAPPER; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** Tests benchmarks. */ +@SuppressWarnings("unchecked") +public class BenchmarkTest extends AbstractTestBase { + @Rule public final TemporaryFolder tempFolder = new TemporaryFolder(); + + private static final String exampleConfigFile = "kmeansmodel-benchmark.json"; + private static final String expectedBenchmarkName = "KMeansModel-1"; + + private final ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + private final PrintStream originalOut = System.out; + + @Before + public void before() { + System.setOut(new PrintStream(outputStream)); + } + + @After + public void after() { + System.setOut(originalOut); + outputStream.reset(); + } + + @Test + public void testCreateAndExecute() throws Exception { + StreamExecutionEnvironment env = StreamExecutionEnvironment.getExecutionEnvironment(); + StreamTableEnvironment tEnv = StreamTableEnvironment.create(env); + + KMeans kMeans = new KMeans().setK(5).setFeaturesCol("test_feature"); + InputDataGenerator<?> inputsGenerator = + new DenseVectorGenerator() + .setColNames("test_feature") + .setNumValues(1000) + .setVectorDim(10); + + BenchmarkResult result = + BenchmarkUtils.runBenchmark(tEnv, "testBenchmarkName", kMeans, inputsGenerator); + + BenchmarkUtils.printResults(result); + assertTrue(outputStream.toString().contains("testBenchmarkName")); + assertTrue(outputStream.toString().contains(result.totalTimeMs.toString())); + } + + @Test + public void testLoadAndSave() throws Exception { Review comment: I am hoping we can simply the test code here and make it more readable. The following logical steps are executed by `flink-ml-benchmark.sh`: - Parse command line argument - Parse the json config file into `Map<String, ?>` - Enumerate the `Map<String, ?>` and call `runBenchmark()` to get a list of BenchmarkResults. - Output the BenchmarkResults to file or stdout. Would it make the code more readable to update Benchmark.java and make its functions logically similar the the steps described above? For example, we can have functions such as `Map<String, ?> parseJsonFile(String path)`. And in general the best practice to test `flink-ml-benchmark.sh` is to test each step separately by testing the corresponding function (e.g. `runBenchmark`, `parseJsonFile`). Also, we might not need to explicitly test the step that converts BenchmarkResult to a string, as the corresponding function is quite straightforward and unlikely to have bug. ########## File path: flink-ml-benchmark/src/test/java/org/apache/flink/ml/benchmark/BenchmarkTest.java ########## @@ -0,0 +1,185 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.ml.benchmark; + +import org.apache.flink.ml.benchmark.data.InputDataGenerator; +import org.apache.flink.ml.benchmark.data.common.DenseVectorGenerator; +import org.apache.flink.ml.clustering.kmeans.KMeans; +import org.apache.flink.ml.clustering.kmeans.KMeansModel; +import org.apache.flink.ml.param.WithParams; +import org.apache.flink.ml.util.ReadWriteUtils; +import org.apache.flink.streaming.api.environment.StreamExecutionEnvironment; +import org.apache.flink.table.api.bridge.java.StreamTableEnvironment; +import org.apache.flink.test.util.AbstractTestBase; + +import org.apache.commons.io.FileUtils; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.InputStream; +import java.io.PrintStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +import static org.apache.flink.ml.util.ReadWriteUtils.OBJECT_MAPPER; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** Tests benchmarks. */ +@SuppressWarnings("unchecked") +public class BenchmarkTest extends AbstractTestBase { + @Rule public final TemporaryFolder tempFolder = new TemporaryFolder(); + + private static final String exampleConfigFile = "kmeansmodel-benchmark.json"; + private static final String expectedBenchmarkName = "KMeansModel-1"; + + private final ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + private final PrintStream originalOut = System.out; + + @Before + public void before() { + System.setOut(new PrintStream(outputStream)); + } + + @After + public void after() { + System.setOut(originalOut); + outputStream.reset(); + } + + @Test + public void testCreateAndExecute() throws Exception { + StreamExecutionEnvironment env = StreamExecutionEnvironment.getExecutionEnvironment(); + StreamTableEnvironment tEnv = StreamTableEnvironment.create(env); + + KMeans kMeans = new KMeans().setK(5).setFeaturesCol("test_feature"); + InputDataGenerator<?> inputsGenerator = + new DenseVectorGenerator() + .setColNames("test_feature") + .setNumValues(1000) + .setVectorDim(10); + + BenchmarkResult result = + BenchmarkUtils.runBenchmark(tEnv, "testBenchmarkName", kMeans, inputsGenerator); + + BenchmarkUtils.printResults(result); Review comment: Do you see any existing example where we check the value by printing it to stdout and check the printed stream? I find it pretty uncommon to do this, which suggests that it is not following the best practice. Would it be simpler to check the value of `result` directly, instead of printing its to stdout and checking the printed String? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
