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

Small cpu_comparison "pythonic" improvements #601

Merged
merged 3 commits into from
Jul 27, 2024

Conversation

makslevental
Copy link
Collaborator

@makslevental makslevental commented Jul 26, 2024

I was gonna start working on #581, which is downstream of one of the comparisons, and I started using the script and then got distracted.

@makslevental makslevental force-pushed the makslevental/vectorizationfixes branch 3 times, most recently from 2fe648a to 9896a40 Compare July 26, 2024 21:17
@makslevental makslevental marked this pull request as ready for review July 26, 2024 21:44
@makslevental makslevental requested review from newling and yzhang93 July 26, 2024 21:44
@makslevental makslevental force-pushed the makslevental/vectorizationfixes branch 3 times, most recently from e5c7e6b to 718ef86 Compare July 26, 2024 22:05
@makslevental makslevental changed the title Small cpu_comparison "pythonic" fixes Small cpu_comparison "pythonic" improvements Jul 26, 2024
Copy link
Contributor

@newling newling left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements. n_repeats vs num_repeats, not clear what's going on is num-repeats a global default?

Please run black if you haven't already

build_tools/ci/cpu_comparison/run_test.py Outdated Show resolved Hide resolved
@makslevental
Copy link
Collaborator Author

Thanks for the improvements. n_repeats vs num_repeats, not clear what's going on is num-repeats a global default?

err I'm blind - I didn't notice n_repeats.

Please run black if you haven't already

I have black bound to ctrl+shift+b and I run it compulsively after every edit lol

@makslevental
Copy link
Collaborator Author

makslevental commented Jul 26, 2024

not clear what's going on is num-repeats a global default?

Since I didn't realize n_repeats existed, I patched in a different semantic for num_repeats (repeating only the run). But actually the current/existing semantic you have for n_repeats is the right one for this test. So I'm gonna remove the changes associated with num_repeats.

@makslevental makslevental force-pushed the makslevental/vectorizationfixes branch from 718ef86 to f0aa863 Compare July 26, 2024 23:38
@makslevental makslevental requested a review from newling July 26, 2024 23:39
@makslevental makslevental force-pushed the makslevental/vectorizationfixes branch from f0aa863 to 4e642ff Compare July 26, 2024 23:40
@newling
Copy link
Contributor

newling commented Jul 27, 2024

not clear what's going on is num-repeats a global default?

Since I didn't realize n_repeats existed, I patched in a different semantic for num_repeats (repeating only the run). But actually the current/existing semantic you have for n_repeats is the right one for this test. So I'm gonna remove the changes associated with num_repeats.

I was wondering about this actually, repeating the compile does hurt test time quite badly and I'm not sure it's worth it. Ideally we could have the option for both, and if we're repeating the compile it'd be nice if we could actually diff the artifacts (xclbin?) to see if compilation is deterministic. xclbin might contain timestamps or something which this not possible though.

TL;DR: my current thinking is that if we have to choose one or the other, we shouldn't be repeating the compilation, just the run... FFS.

Copy link
Contributor

@newling newling left a comment

Choose a reason for hiding this comment

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

n_repeats is still confusing: run_test takes as 2 arguments for the number of repeats, one via config (config.n_repeats) and one via n_repeats.

In this PR, can you please remove all changes to n_repeats, and we can get back to it later? Everything else looks good to me

@makslevental makslevental force-pushed the makslevental/vectorizationfixes branch from aa4c53c to 0758cb4 Compare July 27, 2024 20:11
@makslevental makslevental requested a review from newling July 27, 2024 20:11
@makslevental
Copy link
Collaborator Author

@newling all set

Copy link
Contributor

@newling newling left a comment

Choose a reason for hiding this comment

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

Thanks!

@makslevental makslevental merged commit 3d6cc91 into main Jul 27, 2024
2 checks passed
@makslevental makslevental deleted the makslevental/vectorizationfixes branch July 27, 2024 20:24
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.

2 participants