-
Notifications
You must be signed in to change notification settings - Fork 3
Add init_worker_code for code that runs once per worker
#85
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
base: main
Are you sure you want to change the base?
Conversation
| const init_code = quote | ||
| # Import the previously-defined test function | ||
| # into the temporary test module | ||
| import Main: common_test_helper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know Valentin won't agree with me, but I'm growing allergic to import, lately I've seen a few silent bugs because of using import (which is effectively a spooky action at a distance)
| import Main: common_test_helper | |
| using Main: common_test_helper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it may be useful to explain in the docs why you need to reference Main, the different modules involved when testing with ParallelTestRunner can be confusing (#68)
| function common_test_helper(x) | ||
| return x * 2 | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be useful, but I'm not sure this example illustrates the use case very clearly: I wouldn't expect a function definition to take long time. Or is it that you're recompiling the same code (and in this example is a trivial code anyway) all over again in all tests, because they are technically different function in different modules?
| @testset "init worker code" begin | ||
| init_worker_code = quote | ||
| using Test | ||
| import Main: should_be_defined, @should_also_be_defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing what this is doing here.
|
We did intentionally removed this functionality at some point during a refactor. Would it make sense to move the GPUArraysTest instead into a package? |
With the idea being that a separate package would be precompiled so loading the precompiled test suite for every test would introduce less overhead? I think that would add a significant amount of friction for contributing to GPUArrays. Maybe I de-emphasize it in the docs and make clear that |
The GPUArrays testsuite takes at least 3s to load. It saves a lot of time to only load it once per worker instead of once per test especially in the memory constrained runners that are 7-8GB Mac Minis
Non-breaking