Prune PT matrix based on impacted features calculated from GIBs impacted modules #10984

MiguelWeezardo · 2022-02-08T15:25:24Z

Description

This PR removes unnecessary jobs from the PT matrix based on the impacted features.

General information

This is a performance improvement over starting all PT environments only to have them skip tests later.
This change affects product test runner and GitHub CI jobs.

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

electrum

This will be a nice improvement

electrum · 2022-07-25T05:47:21Z

...-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/cli/SuiteDescribe.java

+        TEXT(TextOutputBuilder::new),
+        JSON(JsonOutputBuilder::new);
+
+        final Supplier<OutputBuilder> outputBuilderFactory;


Make private and add a getter

Done! Getter was not needed since it's only referenced here.

electrum · 2022-07-25T05:48:53Z

...-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/cli/SuiteDescribe.java

+        @Override
+        public void addTestRun(EnvironmentOptions environmentOptions, TestRun.TestRunOptions runOptions, Environment environment)
+        {
+            JsonSuite suite = output.getSuites().get(output.getSuites().size() - 1);


Iterables.getLast()

electrum · 2022-07-25T05:52:45Z

...uncher/src/main/java/io/trino/tests/product/launcher/cli/suite/describe/json/JsonOutput.java

+public class JsonOutput
+{
+    private final String config;
+    private List<JsonSuite> suites = new ArrayList<>();


Using a mutable object for serialization isn't how we normally write code in Trino. Instead, make this object immutable, and in the builder, keep track of the config name and list separately, then build the immutable JsonOutput object in the build() call.

electrum · 2022-07-25T05:53:39Z

...-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/cli/SuiteDescribe.java

+    {
+        private static final JsonCodec<JsonOutput> CODEC = new JsonCodecFactory().jsonCodec(JsonOutput.class);
+
+        private JsonOutput output;


Keep the config name and suite list separate here and build an immutable JsonOutput in the build() call.

electrum · 2022-07-25T05:54:25Z

...uncher/src/main/java/io/trino/tests/product/launcher/cli/suite/describe/json/JsonOutput.java

+        this.config = config;
+    }
+
+    @JsonGetter


Use @JsonProperty. Same for the other classes.

electrum · 2022-07-25T05:55:23Z

...no-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/env/Environment.java

+    @JsonGetter
+    public List<String> getFeatures()
+    {
+        return Stream.of(


Why do we only care about these two features?

These are the only two recognized by TestRun.hasImpactedFeatures() and the only ones that Environments can mark as configured. For any other feature, all tests will always be executed.

electrum · 2022-07-25T05:56:36Z

.github/bin/build-pt-matrix-from-impacted-connectors.py

+            logging.debug("Parsing JSON: %s", line)
+            ptl_output = json.loads(line)
+            logging.debug("Handling JSON object: %s", ptl_output)
+            config_features = {(config, suite.get("name")): set([connector for testRun in suite.get("testRuns", []) for connector in testRun["environment"].get("features", [])])


Try to add some wrapping for readability

The black Python formatter formats this as:

config_features = { (config, suite.get("name")): set( [ connector for testRun in suite.get("testRuns", []) for connector in testRun["environment"].get("features", []) ] ) for suite in ptl_output.get("suites", []) }

but I rewrote this into regular loops so it looks like:

config_features = {} for suite in ptl_output.get("suites", []): key = (config, suite.get("name")) value = set() for testRun in suite.get("testRuns", []): for connector in testRun["environment"].get("features", []): value.add(connector) config_features[key] = value

I think this is more readable than nested list comprehension.

electrum · 2022-07-25T06:00:21Z

.github/workflows/ci.yml

+            - 11
+          exclude:
+            - config: cdh5
+              ignore exclusion if: >-


These ignore exclusion if don't seem to be used in the Python code.

They don't have to be explicitly handled. Because we render the matrix yaml in the ci.yml file, the GHA expressions should be evaluated, so there should only be True/False values. All matrix values will be combined when expanding in the expand_matrix function. There are also tests for that.

nineinchnick · 2022-07-25T08:05:03Z

@electrum thanks for the review! I addressed all comments but I can't resolve them, PTAL.

MiguelWeezardo · 2022-08-30T13:09:40Z

@electrum Could you take another look? Is this merge-worthy?

This will prevent us from accidentally commiting stale versions.

Download JAR used by ptl Make sure errors in ptl suite describe don't prune entire PT matrix Fix fetching of impacted connectors

Instead, unnecessary PTs are: * added to the exclude list, * filtered from the include list The matrix is then expanded by GitHub's own code.

MiguelWeezardo · 2022-10-05T11:45:41Z

@electrum Could you take another look at this? I'd like to get this PR merged, or at least get some feedback on what is missing.

cla-bot bot added the cla-signed label Feb 8, 2022

MiguelWeezardo added the WIP label Feb 8, 2022

github-actions bot added the tests:hive label Feb 8, 2022

MiguelWeezardo force-pushed the gib-prune-pt-matrix branch 14 times, most recently from edf9109 to f534b18 Compare February 11, 2022 21:14

MiguelWeezardo removed the tests:hive label Feb 11, 2022

MiguelWeezardo force-pushed the gib-prune-pt-matrix branch from f534b18 to dabb852 Compare February 11, 2022 21:49

github-actions bot added the tests:hive label Feb 11, 2022

MiguelWeezardo force-pushed the gib-prune-pt-matrix branch 8 times, most recently from e2b53b8 to 76aa5e2 Compare February 18, 2022 10:45

MiguelWeezardo force-pushed the gib-prune-pt-matrix branch 2 times, most recently from 155983b to 0b148e3 Compare February 23, 2022 15:25

MiguelWeezardo force-pushed the gib-prune-pt-matrix branch from 8e39309 to 92d7ad8 Compare May 11, 2022 12:48

electrum reviewed Jul 25, 2022

View reviewed changes

nineinchnick force-pushed the gib-prune-pt-matrix branch from 92d7ad8 to 5295cab Compare July 25, 2022 08:03

nineinchnick force-pushed the gib-prune-pt-matrix branch 2 times, most recently from 7f0b2b9 to 9896107 Compare August 24, 2022 10:33

MiguelWeezardo requested a review from electrum August 30, 2022 12:57

MiguelWeezardo added 6 commits October 5, 2022 11:53

Add temporary pipeline files to .gitignore

7d46b3a

This will prevent us from accidentally commiting stale versions.

Add environment feature info in JSON format to ptl suite describe

ad9c13b

Add a job which generates the PT matrix

fceebbf

Download JAR used by ptl Make sure errors in ptl suite describe don't prune entire PT matrix Fix fetching of impacted connectors

Use constants from PluginReader for feature prefixes in ptl

9f1983f

Disable feature-based filtering of PT test matrix if PTs changed

a451df2

Avoid rewriting PT matrix into a flat include list

3d3f826

Instead, unnecessary PTs are: * added to the exclude list, * filtered from the include list The matrix is then expanded by GitHub's own code.

MiguelWeezardo force-pushed the gib-prune-pt-matrix branch from 9896107 to 3d3f826 Compare October 5, 2022 09:58

MiguelWeezardo requested review from Praveen2112 and removed request for martint, wendigo, kokosing and bitweld October 6, 2022 09:48

electrum approved these changes Oct 8, 2022

View reviewed changes

electrum merged commit c43967a into trinodb:master Oct 8, 2022

github-actions bot added this to the 400 milestone Oct 8, 2022

This was referenced Oct 9, 2022

Fix product test filtering on master #14534

Merged

Always run all product tests on master #14541

Merged

colebow mentioned this pull request Oct 11, 2022

Add Trino 400 release notes #14556

Merged

nineinchnick mentioned this pull request Oct 17, 2022

Fix ci outputs #14662

Merged

MiguelWeezardo deleted the gib-prune-pt-matrix branch October 18, 2022 08:15

ebyhr mentioned this pull request Oct 19, 2022

Don't run testTrinoPreservesReaderAndWriterVersions in Databricks 7.3 #14687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prune PT matrix based on impacted features calculated from GIBs impacted modules #10984

Prune PT matrix based on impacted features calculated from GIBs impacted modules #10984

MiguelWeezardo commented Feb 8, 2022 •

edited

Loading

electrum left a comment

electrum Jul 25, 2022

nineinchnick Jul 25, 2022

electrum Jul 25, 2022

electrum Jul 25, 2022

electrum Jul 25, 2022

electrum Jul 25, 2022

electrum Jul 25, 2022

nineinchnick Jul 25, 2022

electrum Jul 25, 2022

nineinchnick Jul 25, 2022

electrum Jul 25, 2022

nineinchnick Jul 25, 2022

nineinchnick commented Jul 25, 2022

MiguelWeezardo commented Aug 30, 2022 •

edited

Loading

MiguelWeezardo commented Oct 5, 2022

Prune PT matrix based on impacted features calculated from GIBs impacted modules #10984

Prune PT matrix based on impacted features calculated from GIBs impacted modules #10984

Conversation

MiguelWeezardo commented Feb 8, 2022 • edited Loading

Description

General information

Related issues, pull requests, and links

Documentation

Release notes

electrum 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nineinchnick commented Jul 25, 2022

MiguelWeezardo commented Aug 30, 2022 • edited Loading

MiguelWeezardo commented Oct 5, 2022

MiguelWeezardo commented Feb 8, 2022 •

edited

Loading

MiguelWeezardo commented Aug 30, 2022 •

edited

Loading