Skip to content

Conversation

@austin3dickey
Copy link
Contributor

@austin3dickey austin3dickey commented Apr 5, 2024

Fixes #41015. This PR allows users to run yarn perf to run the JS benchmarks on more machines; in particular, machines that do not support env -S like Amazon Linux.

@github-actions
Copy link

github-actions bot commented Apr 5, 2024

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

@austin3dickey
Copy link
Contributor Author

@ursabot please benchmark lang=JavaScript

@ursabot
Copy link

ursabot commented Apr 5, 2024

Benchmark runs are scheduled for commit e5f6c93. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, now we have the same command (node --no-warnings --loader ts-node/esm/transpile-only) in two places (the package.json and the ts file). Where else do we call perf from? Could we just change all the calls to run yarn perf / npm run perf?

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 5, 2024
@austin3dickey
Copy link
Contributor Author

Where else do we call perf from? Could we just change all the calls to run yarn perf / npm run perf?

As far as I know, everything that runs benchmarks just uses yarn perf.

Now we have the same command in two places

If this is a problem, perhaps we should remove the shebang then? The trouble with the shebang as written is that using /usr/bin/env -S is non-portable. So if we're using yarn perf everywhere, it should be fine to remove the shebang.

@domoritz
Copy link
Member

domoritz commented Apr 5, 2024

Sounds good to me. We have a few other places that use the same shebang and I think we should update them as well.

Rather than yarn, maybe we should use npm run since it's more portable. But that can be a separate change.

@austin3dickey
Copy link
Contributor Author

We have a few other places that use the same shebang and I think we should update them as well.

Just looked into this briefly and found places in archery that might fail if we remove the shebangs:

_VALIDATE = os.path.join(_EXE_PATH, 'integration.ts')
_JSON_TO_ARROW = os.path.join(_EXE_PATH, 'json-to-arrow.ts')
_STREAM_TO_FILE = os.path.join(_EXE_PATH, 'stream-to-file.ts')
_FILE_TO_STREAM = os.path.join(_EXE_PATH, 'file-to-stream.ts')

So we could go through and fix all these, but it might be more trouble than it's worth. Would you mind if I keep this PR scoped to just fixing the benchmarks' portability?

(For context, I'm trying to move the orchestration of the Arrow merge-commit JS benchmarks off a machine in someone's basement to an EC2 instance. This PR unblocks that work.)

@domoritz
Copy link
Member

domoritz commented Apr 5, 2024

So we could go through and fix all these, but it might be more trouble than it's worth. Would you mind if I keep this PR scoped to just fixing the benchmarks' portability?

Agreed. So to unblock you, can you run node --no-warnings --loader ts-node/esm/transpile-only perf/index.ts from the perf script? Otherwise, sure, let's merge this as is. I approve either way.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Apr 5, 2024
@conbench-apache-arrow
Copy link

Thanks for your patience. Conbench analyzed the 1 benchmarking run that has been run so far on PR commit e5f6c93.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@austin3dickey
Copy link
Contributor Author

I think that might be slightly difficult on the orchestration side. Cool, thanks for the review. 💜 would you mind merging? I don't have write access.

@domoritz domoritz merged commit 02bc653 into apache:main Apr 5, 2024
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 02bc653.

There was 1 benchmark result indicating a performance regression:

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

vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[JS] [Benchmarking] JavaScript benchmarks don't work on Amazon Linux

3 participants