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

Broken benchmark check on CI #15089

Closed
jtpio opened this issue Sep 7, 2023 · 2 comments · Fixed by #15523
Closed

Broken benchmark check on CI #15089

jtpio opened this issue Sep 7, 2023 · 2 comments · Fixed by #15523

Comments

@jtpio
Copy link
Member

jtpio commented Sep 7, 2023

Description

The "Benchmark Tests" CI check fails consistently when PRs are approved:

image

Reproduce

Example run: https://github.com/jupyterlab/jupyterlab/actions/runs/6043794173/job/16401409899?pr=15042

Expected behavior

The benchmark check should pass most of the time.

Context

Do we still need to run the "Benchmark Tests" on PR approval? This triggers a 1h long CI job which adds CI fatigue on maintainers who might not be sure whether this check is important or not.

Also it looks like maintainers are now ignoring this check when merging PRs (since it's consistently failing), so this raises the question about it is really useful.

@jtpio jtpio added bug status:Needs Triage Applied to new issues that need triage labels Sep 7, 2023
@jtpio
Copy link
Member Author

jtpio commented Sep 7, 2023

Looks like this may be fixable if the only issue is about publishing the report:

Warning: "cml publish" is deprecated since "cml comment" now supports "![inline](./asset.png)"

image

@krassowski
Copy link
Member

krassowski commented Sep 7, 2023

We could just use GitHub actions instead of cml, as in jupyterlab/benchmarks#144. That supports markdown so if we generate a PNG we can base64 encode it.

Or maybe we could just post the result as a comment using gh?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants