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

Hook the benchmark runner and benchmark visualizations into GitHub #30

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

petervdonovan
Copy link
Contributor

@petervdonovan petervdonovan marked this pull request as ready for review July 19, 2022 16:31
@cmnrd
Copy link
Contributor

cmnrd commented Jul 22, 2022

I think this needs to be rebased to (or merged with) master now after #2 was merged.

@@ -10,8 +10,8 @@
import json


def dir_path(string):
if os.path.isdir(string):
def src_path_type(string):
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't change anymore right? Since dir_path now is an optional argument and we don't explicitly use 'latest'



def execute_command(command, timeout=None, stacktrace=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you provide some context on why you changes this and what exactly it does?

Copy link
Contributor Author

@petervdonovan petervdonovan Jul 26, 2022

Choose a reason for hiding this comment

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

I wanted to read the output of the process in a non-blocking way so that we could capture the stack trace of the running process before killing it. By default, the stack trace is captured because the user may not have eu-stack, but I wanted this feature so that ideally, it might be possible to debug a deadlock in CI without having to manually run the executable hundreds of times.

@petervdonovan petervdonovan marked this pull request as draft October 4, 2022 05:54
@lhstrh
Copy link
Member

lhstrh commented Sep 24, 2023

This looks like it would be super useful, but it's somehow still marked as draft. How close to being merged was this when it got stalled?

@petervdonovan
Copy link
Contributor Author

petervdonovan commented Sep 24, 2023

I do not remember, but it was pretty close. I did not merge it partly because I do not have the time to make the behavior of the changes acceptable to everyone, but also because I do not have the time to take advantage of the outputs that the changes would give if they were merged. Performance optimization for the C target is not among my near-term goals, and if someone else does take that up as a near-term goal (I am not sure that they should), then they can do what they think is necessary in order to guard against performance regressions.

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

Successfully merging this pull request may close these issues.

3 participants