Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-37657: [JS] Run bin scripts with ts-node #37668

Closed
wants to merge 8 commits into from

Conversation

trxcllnt
Copy link
Contributor

@trxcllnt trxcllnt commented Sep 11, 2023

  • update bin scripts to use ts-node in transpile-only mode
  • remove ts-node config from top level tsconfig.json
  • fix tsc errors in bin scripts

Fixes #37657

…s to use ts-node in transpile-only mode, fix tsc errors in bin scripts
@github-actions
Copy link

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

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.

Looks good overall. Eventually, we could even try running the benchmarks in e.g. bun so that we also have jscore as a comparison and not just v8 via node.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Sep 12, 2023
@kou
Copy link
Member

kou commented Sep 12, 2023

It seems that integration CI job failure is related to this: https://github.com/apache/arrow/actions/runs/6152750564/job/16695430388?pr=37668

Could you check the log?

@austin3dickey
Copy link
Contributor

I just noticed that this open PR is expected to close #37657. That's great news! Could someone please take another look to see if we can merge and turn the benchmarking jobs green again?

@kou
Copy link
Member

kou commented Oct 11, 2023

@trxcllnt Could you rebase on main and resolve conflict?

@domoritz Could you review this again after that?

@domoritz
Copy link
Member

I'm happy to do another review.

@raulcd
Copy link
Member

raulcd commented Oct 18, 2023

Hi, from my understanding benchmarks haven't been run for the last month or so for JS. It would be nice to re-enable them to validate we are not adding any performance issue on the Release.

@austin3dickey
Copy link
Contributor

@ursabot please benchmark lang=JavaScript

@ursabot
Copy link

ursabot commented Oct 26, 2023

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

@conbench-apache-arrow
Copy link

Thanks for your patience. Conbench analyzed the 0 benchmarking runs that have been run so far on PR commit 2ee36ea.

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

@austin3dickey
Copy link
Contributor

I checked out this branch locally, rebased to main, and the JavaScript benchmarks work again. I don't think I have access to push to this fork, but that's a great sign! Thanks for the work so far.

If we don't merge this PR soon, I'm going to turn off the automatic benchmarking for JS since it's failing every time and covering up other errors like voltrondata-labs/arrow-benchmarks-ci#166.

@assignUser
Copy link
Member

I have rebased the PR locally favoring main for the yarn.lock conflicts. In the interest of getting the benchmarks going again I have opened an alternative PR #38500 (keeping authorship for @trxcllnt of course).

assignUser added a commit that referenced this pull request Nov 2, 2023
(rebased version of #37668, yarn.lock conflicts resolved in favor of main)
* update bin scripts to use ts-node in transpile-only mode
* remove ts-node config from top level tsconfig.json
* fix tsc errors in bin scripts
* Closes: #37657

Authored-by: ptaylor <paul.e.taylor@me.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
@raulcd
Copy link
Member

raulcd commented Nov 7, 2023

Should we close this PR as the other one has been merged?

@assignUser
Copy link
Member

Yep, sorry I forgot to do that.

@assignUser assignUser closed this Nov 7, 2023
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
(rebased version of apache#37668, yarn.lock conflicts resolved in favor of main)
* update bin scripts to use ts-node in transpile-only mode
* remove ts-node config from top level tsconfig.json
* fix tsc errors in bin scripts
* Closes: apache#37657

Authored-by: ptaylor <paul.e.taylor@me.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
(rebased version of apache#37668, yarn.lock conflicts resolved in favor of main)
* update bin scripts to use ts-node in transpile-only mode
* remove ts-node config from top level tsconfig.json
* fix tsc errors in bin scripts
* Closes: apache#37657

Authored-by: ptaylor <paul.e.taylor@me.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
@trxcllnt trxcllnt deleted the fix/ts-node-configs branch April 19, 2024 00:23
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 microbenchmarks are failing
7 participants