-
Notifications
You must be signed in to change notification settings - Fork 962
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
How to run tests with the Panama Vector implementation #13344
Comments
/cc @uschindler |
Could it be the ";" in your command line? |
wah you're right. It's not the export. The reason for that is; During testing we set arbitrary bit sizes, but this will slow you down immense so it disables the panama provider explicit. This was added by Robert at some point. The current code is fine, but theres no way to run all tests with the default provider..... Let me think about this. We have to find some way to execute tests "under real conditions". |
Might be that we allow the default sizes/values to be set, so e.g. 128 bit on Mac, etc. without disabling the Panama provider. |
I think there should be the option to run this with defaults. |
I think we must do some magic to not set those: lucene/gradle/testing/randomization.gradle Lines 108 to 113 in 40cae08
This is all bad. The problem is that those settings are static and we can't have a per-instance seeting, so it affects both test mode and normal mode. As running all tests would be incredibly slow with wrong settings (non-default), we use default provider for all tests except those with use testMode. |
see #12681 |
Those settings are really only useful when running the test which compares that the Panama and default implementations emit the same results. If we could somehow just limit setting them to that test!! |
That's exactly the problem. According to Robert's testing, the static final constants were better for performance. |
No idea yet, still thinking about it! |
I really wanna spawn a new JVM with those options for a small set of tests, rather than have them infect all other running tests. For example, in the JDK test harness, jtreg, this is trivial - just add |
There are two possibilities how to do this: Spawn the tests that use testMode in a child VM or we add a separate sourceset with tests that run with hardcoded bitsizes. Maybe for the few tests we can use one of the "child spawner" test examples which do this already. |
I wonder if we can just avoid some of this complexity. For example, locally I can run all the tests with the Panama Vector implementation, by doing this:
( setting an empty string for vectorsize results in the default vector size for the platform ) What we want to support is to allow for testing Panama Vector with different sizes during development of the code, so basically this:
If we just flip the defaults of these properties, vectorsize to <empty_string> and forceintegervectors to false, then all tests will use Panama Vector as appropriate - same as the "real world", while not hurting the development time usage. So the concrete proposal is to just set the default value of vectorsize to <empty_string> and forceintegervectors to false. |
We can change the defaults, sure. I am just afraid that Robert will be annoyed. He wants to randomly test all bit sizes. By the way the empty string is allowed because of this: lucene/lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java Line 50 in 30da7da
My quickest idea would be to create dynamically 3 test Gradle tasks with correct config (similar like we do for beasting). I can try it out. Those tasks would just run the testMode=true tests in a separate task like beasting also does. |
Yes, exactly. I'm exploiting this, which is a fine way to say "default" :-)
That would be awesome. So flip the properties back to defaults "real world" for the majority of tests, and add a few targeted gradle tasks that run TestVectorUtilSupport. Sounds good to me. @uschindler I'll leave this to you since you seem to be more familiar with the infrastructure. But I'm happy to "have a go at it" if you want. |
@uschindler your idea above is clearly preferable, but another alternative is to just add <empty_string> to the set of values in |
Hi, sorry for being late. I am busy at moment, but as quick fix I applied locally the following: gradle/testing/randomization.gradle | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/gradle/testing/randomization.gradle b/gradle/testing/randomization.gradle
index 44ae964cab0..f3d25d0c4e2 100644
--- a/gradle/testing/randomization.gradle
+++ b/gradle/testing/randomization.gradle
@@ -105,12 +105,8 @@ allprojects {
[propName: 'tests.LUCENE_VERSION', value: baseVersion, description: "Base Lucene version."],
[propName: 'tests.bwcdir', value: null, description: "Data for backward-compatibility indexes."],
// vectorization related
- [propName: 'tests.vectorsize',
- value: { ->
- RandomPicks.randomFrom(new Random(projectSeedLong), ["128", "256", "512"])
- },
- description: "Sets preferred vector size in bits."],
- [propName: 'tests.forceintegervectors', value: "true", description: "Forces use of integer vectors even when slow."],
+ [propName: 'tests.vectorsize', value: null, description: "Sets preferred vector size in bits."],
+ [propName: 'tests.forceintegervectors', value: false, description: "Forces use of integer vectors even when slow."],
]
}
} This restores the old defaults, but still hsows all parameters in documentation. |
P.S.: I had the same idea to enable it randomly by adding another item to the random list. Next to that, maybe the best would be to have another option to do two things:
This would allow to run all tests under real conditions, e.g. like |
I opened the above PR #13351, which implements your suggestion @uschindler. We can consider that independently, of the possibility of adding specific test tasks for different bit sizes of TestVectorUtilSupport - which I would love to have eventually. |
Hi, thats a fine PR but it still has some problem, so I'd tend to move away from "devlopers" using CI=true and instead use another setting. Will explain in the PR. |
I previously used to do:
but this is not enough, in the output we see:
because either or both of
tests.vectorsize
ortests.forceintegervectors
is set.The text was updated successfully, but these errors were encountered: