feat: add pre- and post-benchmark scripts#93
Conversation
not-matthias
force-pushed
the
cod-1010-benchmark-setup-is-slow-when-running-with-isolated-cores
branch
from
3986f87 to
cbba43e
Compare
art049
left a comment
art049
left a comment
There was a problem hiding this comment.
Instead of relying on env variables maybe we could have a set of default path to check?
Just suggesting this because we have quite a lot of env variables already and avoiding adding new ones to think about is good at this point IMO.
Maybe we could also add a CLI lag to opt-out of those pre/post scripts as well if that's needed. WDYT?
not-matthias
force-pushed
the
cod-1010-benchmark-setup-is-slow-when-running-with-isolated-cores
branch
from
7669674 to
194d890
Compare
not-matthias
force-pushed
the
cod-1008-benchmark-run-fails-on-macro-runner-with-systemd-run
branch
from
868f7d9 to
e3afd62
Compare
not-matthias
force-pushed
the
cod-1010-benchmark-setup-is-slow-when-running-with-isolated-cores
branch
2 times, most recently
from
41784aa to
ec3413f
Compare
good idea!
I'm a bit torn whether to add this. On the one hand we also don't want to add too many CLI args and I'm not sure how useful this one would be. When running locally the scripts won't be executed if they don't exist. The only case where it would be interesting would be when there's issues with the isolation. But I would argue it's better to fix this by changing the pre/post bench scripts in the macro runner. |
art049
left a comment
art049
left a comment
There was a problem hiding this comment.
exceppt the strange test change, lgtm
| #[test] | ||
| fn test_venv_compat_no_crash() { | ||
| assert!(symlink_libpython(None).is_ok()); | ||
| let _ = symlink_libpython(None); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ahh, yeah. Probably should've created a linear issue/separate PR for this. On my system this test failed because libpython is statically linked. I've created a separate commit and wanted to include it in this PR, but I guess it's better to create a new one.
There was a problem hiding this comment.
Ah, okay, well, we still need to keep this test, though
There was a problem hiding this comment.
Ah, okay, well, we still need to keep this test, though
Yeah, definitely. But we can't really check if we patched it since there's no way to ensure that libpython is dynamically linked. So my intention here was to just check if it ran, and didn't crash.
There was a problem hiding this comment.
maybe we can have something to skip it locally and run it still in the CI?
https://nexte.st/docs/running/
There was a problem hiding this comment.
Or https://crates.io/crates/test-with only when CI is set
There was a problem hiding this comment.
Fixed it. We're now only running it in CI using the test-with create (nice find!). Is it fine to keep it in this PR, feels a bit overkill to create a new one/ticket for this.
not-matthias
force-pushed
the
cod-1010-benchmark-setup-is-slow-when-running-with-isolated-cores
branch
from
99edace to
e73c80b
Compare
not-matthias
force-pushed
the
cod-1008-benchmark-run-fails-on-macro-runner-with-systemd-run
branch
from
e3afd62 to
79b3ae1
Compare
not-matthias
force-pushed
the
cod-1010-benchmark-setup-is-slow-when-running-with-isolated-cores
branch
from
e73c80b to
fa91580
Compare
not-matthias
merged commit fa91580
into
cod-1008-benchmark-run-fails-on-macro-runner-with-systemd-run
not-matthias
deleted the
cod-1010-benchmark-setup-is-slow-when-running-with-isolated-cores
branch
2 participants
This allows us to run custom logic on the macro runner (e.g. set core isolation) without having all of that logic inside the runner.