GH-44062: [Dev][Archery][Integration] Reduce needless test matrix by kou · Pull Request #44099 · apache/arrow

kou · 2024-09-13T06:30:13Z

Rationale for this change

If we enable C++, Java and Rust, we use the following patterns:

Producer	Consumer
C++	C++
C++	Java
C++	Rust
Java	C++
Java	Java
Java	Rust
Rust	C++
Rust	Java
Rust	Rust

In apache/arrow, the following patterns are redundant because they should be done in apache/arrow-rs:

Producer	Consumer
Rust	Rust

In apache/arror-rs, the following patterns are redundant because they should be done in apache/arrow:

Producer	Consumer
C++	C++
C++	Java
Java	C++
Java	Java

What changes are included in this PR?

Add --target-languages option. We can specify target languages by this. Here are expected usages:

In apache/arrow:

--target-languages=cpp,csharp,go,java,js

In apache/arrow-rs

--target-languages=rust

Here is an example in apache/arrow-rs:

Used matrix:

Producer	Consumer
Rust	Rust
Rust	C++
Rust	C#
Rust	Go
Rust	Java
Rust	JS
Rust	nanoarrow
C++	Rust
C#	Rust
Go	Rust
Java	Rust
JS	Rust
nanoarrow	Rust

If no --target-languages is specified, all enabled languages are the target languages. (The same as the current behavior.)

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

GitHub Issue: [Dev][Archery][Integration] Reduce needless test matrix #44062

If we enable C++, Java and Rust, we use the following patterns: | Producer | Consumer | |----------|----------| | C++ | C++ | | C++ | Java | | C++ | Rust | | Java | C++ | | Java | Java | | Java | Rust | | Rust | C++ | | Rust | Java | | Rust | Rust | In apache/arrow, the following patterns are redundant because they should be done in apache/arrow-rs: | Producer | Consumer | |----------|----------| | Rust | Rust | In apache/arror-rs, the following patterns are redundant because they should be done in apache/arrow: | Producer | Consumer | |----------|----------| | C++ | C++ | | C++ | Java | | Java | C++ | | Java | Java | Add `--target-language` option. We can specify target languages by this. (We can specify `--target-language` multiple times.) Here are expected usages: In apache/arrow: * `--target-language=cpp` * `--target-language=csharp` * `--target-language=go` * `--target-language=java` * `--target-language=js` In apache/arrow-rs * `--target-language=rust` Here is an example in apache/arrow-rs: T: Languages specified by `--target-language` * rust O: Languages not specified by `--target-language` * cpp * csharp * go * java * js * nanoarrow Used matrix: | Producer | Consumer | |----------|----------| | Rust | Rust | | Rust | C++ | | Rust | C# | | Rust | Go | | Rust | Java | | Rust | JS | | Rust | nanoarrow| | C++ | Rust | | C# | Rust | | Go | Rust | | Java | Rust | | JS | Rust | | nanoarrow| Rust |

github-actions · 2024-09-13T06:30:39Z

⚠️ GitHub issue #44062 has been automatically assigned in GitHub to PR creator.

kou · 2024-09-13T07:35:25Z

Hmm. "Rust -> Rust" still exists...:

https://github.com/apache/arrow/actions/runs/10844206629/job/30092647347?pr=44099#step:9:11984

Integration: Test: IPC: Rust -> Rust

kou · 2024-09-13T07:51:19Z

ci/scripts/integration_arrow.sh

    --with-java=$([ "$ARROW_INTEGRATION_JAVA" == "ON" ] && echo "1" || echo "0") \
+    --target-language=java \
    --with-js=$([ "$ARROW_INTEGRATION_JS" == "ON" ] && echo "1" || echo "0") \
+    --target-language=js \


This file is used by apache/arrow-rs too.
So embedding --target-language here isn't a good approach.

kou · 2024-09-14T07:08:11Z

+1

No "Rust -> Rust":

https://github.com/apache/arrow/actions/runs/10858710101/job/30137035840?pr=44099#step:9:6499

Integration: Test: IPC: C++ -> C++
Integration: Test: IPC: C++ -> Java
Integration: Test: IPC: C++ -> JS
Integration: Test: IPC: C++ -> C#
Integration: Test: IPC: C++ -> Go
Integration: Test: IPC: Java -> C++
Integration: Test: IPC: Java -> Java
Integration: Test: IPC: Java -> JS
Integration: Test: IPC: Java -> C#
Integration: Test: IPC: Java -> Go
Integration: Test: IPC: JS -> C++
Integration: Test: IPC: JS -> Java
Integration: Test: IPC: JS -> JS
Integration: Test: IPC: JS -> C#
Integration: Test: IPC: JS -> Go
Integration: Test: IPC: C# -> C++
Integration: Test: IPC: C# -> Java
Integration: Test: IPC: C# -> JS
Integration: Test: IPC: C# -> C#
Integration: Test: IPC: C# -> Go
Integration: Test: IPC: Go -> C++
Integration: Test: IPC: Go -> Java
Integration: Test: IPC: Go -> JS
Integration: Test: IPC: Go -> C#
Integration: Test: IPC: Go -> Go
Integration: Test: IPC: C++ -> Rust
Integration: Test: IPC: Java -> Rust
Integration: Test: IPC: JS -> Rust
Integration: Test: IPC: C# -> Rust
Integration: Test: IPC: Go -> Rust
Integration: Test: IPC: Rust -> C++
Integration: Test: IPC: Rust -> Java
Integration: Test: IPC: Rust -> JS
Integration: Test: IPC: Rust -> C#
Integration: Test: IPC: Rust -> Go

### Rationale for this change #44099 included debug prints. ### What changes are included in this PR? Remove debug prints. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

conbench-apache-arrow · 2024-09-17T07:51:02Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 38b0c79.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

pitrou · 2024-09-17T12:15:28Z

dev/archery/archery/cli.py

+@click.option('--target-languages', default='',
+              help=('Target languages in this integration tests'),
+              envvar="ARCHERY_INTEGRATION_TARGET_LANGUAGES")


Uh, can the help string be more explanatory? It's really not possible to understand what this does from the current wording.

Sure. But is the help string a suitable location to explain it more with click API?
In general, --help output uses a simple one-line (or a few lines) per option.

Then it can be explained in the command's help: https://click.palletsprojects.com/en/8.1.x/api/#click.Command

Thanks. I'll use it: GH-44158

pitrou · 2024-09-17T12:18:20Z

dev/archery/archery/integration/runner.py

+                  run_c_data=False, tempdir=None, target_languages="",
+                  **kwargs):
    tempdir = tempdir or tempfile.mkdtemp(prefix='arrow-integration-')
+    print(["before", target_languages])


You should remove those print calls

Yes... I've removed it: #44140

pitrou · 2024-09-17T12:18:43Z

dev/archery/archery/integration/runner.py

+                  **kwargs):
    tempdir = tempdir or tempfile.mkdtemp(prefix='arrow-integration-')
+    print(["before", target_languages])
+    target_languages = list(filter(len, target_languages.split(",")))


Why the filter call?

Because "".split(",") returns [""] not [].
(I surprised the Python behavior because "".split(",") returns [] with Ruby.)

Why would you pass ,? It's ok to make it an error IMHO.

Because --target-languages=cpp,java is an input. We need to extract cpp and java from cpp,java.

I know that click supports --target-language=cpp --target-languages=java style. But reusing ci/scripts/integration_arrow.sh in apache/arrow, apache/arrow-rs, apache/arrow-nanoarrow and apache/arrow-go is difficult with the style. Entirely overwritable --target-languages=cpp,java style is easy to reusable.

Ah, you're talking about the empty string, sorry, I had misread.
Then I would suggest the much more idiomatic

Suggested change

target_languages = list(filter(len, target_languages.split(",")))

target_languages = target_languages.split(",") if target_languages else []

Thanks. I'll use it in #44156.

pitrou · 2024-09-17T12:19:45Z

dev/archery/archery/cli.py

 @click.option('--with-rust', type=bool, default=False,
              help='Include Rust in integration tests',
              envvar="ARCHERY_INTEGRATION_WITH_RUST")
+@click.option('--target-languages', default='',


"Languages" is not really right here, as nanoarrow for example is not a language. "Implementations" perhaps?

OK. I'll rename it: GH-44155

kou requested review from assignUser, jonkeane and raulcd as code owners September 13, 2024 06:30

github-actions bot added the awaiting committer review Awaiting committer review label Sep 13, 2024

kou commented Sep 13, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 13, 2024

Use string

bad8c35

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 13, 2024

kou added 2 commits September 13, 2024 18:29

Fix a typo

f39ed69

Fix lint

1055eb3

alamb mentioned this pull request Sep 13, 2024

[Dev][Archery][Integration] Reduce needless test matrix #44062

Closed

kou added 4 commits September 14, 2024 07:42

Fix empty string case

0567f1a

Fix a typo

b092bc3

Add a debug print

b3c9cb7

Add missing export

f4fd547

kou merged commit 38b0c79 into apache:main Sep 16, 2024

kou removed the awaiting change review Awaiting change review label Sep 16, 2024

kou deleted the ci-integration-reduce-matrix branch September 16, 2024 23:53

This was referenced Sep 17, 2024

Reduce integration test matrix apache/arrow-rs#6406

Closed

Reduce integration test matrix apache/arrow-rs#6407

Merged

MINOR: [Dev][Archery][Integration] Remove debug prints #44140

Merged

pitrou reviewed Sep 17, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 17, 2024

This was referenced Sep 18, 2024

[Archery][Integration] Rename "language" to "implementation" #44155

Closed

[Archery][Integration] Add more explanation how --target-implementations works #44158

Closed

kou mentioned this pull request Oct 21, 2024

GH-44361: [C#][Integration] Include .NET in Flight integration tests #44377

Merged

	target_languages = list(filter(len, target_languages.split(",")))
	target_languages = target_languages.split(",") if target_languages else []

Comments

Conversation

kou commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Sep 13, 2024

Uh oh!

kou commented Sep 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou commented Sep 14, 2024

Uh oh!

conbench-apache-arrow bot commented Sep 17, 2024

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!

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!

2 participants

kou commented Sep 13, 2024 •

edited

Loading