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

Use tempdir to run test to support pytest parallel #138

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

leonardt
Copy link
Contributor

Addresses #134

@leonardt
Copy link
Contributor Author

leonardt commented Jun 19, 2019

Simple example seems to work, but there's a major issue to resolve: copying the required files per test is wasteful, instead it should be the case that every test thread has their own temporary directory (so we copy once per thread). The issue here is that it doesn't seem like pytest-parallel has an API for getting the thread ID, so using pytest-xdist might be a better option for this. We'll need to setup a test fixture which parametrizes the test directory, which in turn should use the xdist API or ENV var to get the thread id, create the tempdir, and copy the files over. Also, upon failure, the test thread should not clean up it's tempdir so the user can see the output collateral for debuggging (e.g. inspect the generated test bench). Maybe this is not a big issue, because in the single threaded model we're clobbering the generated test bench file anyways.

Also, it would be nice to preserve the old behavior when running in a single thread.

@leonardt
Copy link
Contributor Author

Also the buildkite run failed but didn't set the exit code properly for some reason, see https://buildkite.com/stanford-aha/lassen/builds/291#ca69d244-5d2e-4414-8eae-7e01ecbdf248

@leonardt
Copy link
Contributor Author

ah, the issue was that the error was occuring during test collection

@cdonovick
Copy link
Contributor

cdonovick commented Jun 19, 2019

@leonardt Is there a reason we need to copy and not symlink?
If there is I think I have idea on how to fix this. We could create a pool of directories. Tests would first try to get a directory from the pool and if one isn't available set up a new one. I'll take a stab it tomorrow.
This would effectively continue to preserve the old behavior in single threaded mode.

@leonardt
Copy link
Contributor Author

I don't think symlinking would necessarily work because the threads would then be sharing obj_dir/INCA_LIBS which are the collateral directories used by verilator/ncsim. Each thread would be generating/compiling their own testbench, so there would likely be some sort of conflict in these shared collateral directories when the simulator is emitting new collateral for each test bench

@leonardt
Copy link
Contributor Author

@cdonovick using a directory pool seems like a reasonable approach, I'm working out some kinks with the travis/buildkite builds, let me know if you need help understanding any of the test facilities, but it should be pretty clear where the tempdir stuff is and where you can replace it with your logic.

@leonardt
Copy link
Contributor Author

Ok, travis/buildkite are green, seems like it's working although the stdout is a bit funky, I suspect -v was not intended to be used with parallel, the travis build didn't seem to speed up by much (probably because it's only giving 2 threads and the VM probably is using a single physical thread). The buildkite build does speedup to 7m from 18m (last master build) so that's promising.

I'm a little paranoid that we somehow borked the test logic, so it could be worth throwing some poison pills (e.g. purposefully mess up some instructions in the ALU in a separate branch) to make sure that the tests are still catching errors properly.

@leonardt
Copy link
Contributor Author

Actually for travis the pytest invocation runtime dropped by ~70s which is actually decent. It's runnign less tests than buildkite (so benefits less from parallel speedup) and actually is mostly dominated by the VM setup time (travis bootup scripts, installing dependencies, etc...)

@leonardt
Copy link
Contributor Author

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