-
Notifications
You must be signed in to change notification settings - Fork 11
Fix Tests/CI, refactor merge to call CAGRA's merge(), implement CAGRA prefiltering #14
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
Changes from all commits
4367f36
f136334
283e2cf
e19b628
2d96c71
53d326c
3f0a03b
433c485
84eae2f
9498678
40e21f1
31f1dab
6f8bebd
5692496
41a0124
221de3d
5de9cc9
00947ef
3d9de2d
5b1313b
5b90792
6c0fb70
1b9ce64
f0efa86
4b09035
8c8e009
c4f1abb
fa7cdb1
f4bc2e6
06e39dd
76010c0
d8bb38d
f27f907
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,6 +86,12 @@ public CuVSVectorsFormat( | |
| } | ||
|
|
||
| private static CuVSResources cuVSResourcesOrNull() { | ||
| try { | ||
| System.loadLibrary( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you can pass arguments to loadLibrary in your Java code and not rely on LD_LIBRARY_PATH, things will probably be more robust. The hard part is that it must be recursive. Loading a conda library should load all conda dependencies, or else you get weird runtime errors. This is especially problematic with libstdc++. One way to ensure that libstdc++ does not accidentally come from the system is to explicitly load it from the conda environment before loading any other (conda-based) libraries. I don't know much at all about Java, but this site seemed to have some helpful alternatives to LD_LIBRARY_PATH. The more you can change just your process and not the environment, the fewer strange issues you'll have. LD_LIBRARY_PATH is a foot gun. Use it if you must, but only after exhausting other options. |
||
| "cudart"); // nocommit: this is here so as to pass CI, should goto cuvs-java | ||
| } catch (UnsatisfiedLinkError e) { | ||
| LOG.warning("Could not load CUDA runtime library: " + e.getMessage()); | ||
| } | ||
| try { | ||
| resources = CuVSResources.create(); | ||
| return resources; | ||
|
|
||
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.
We need to be careful because this could tie our release packaging to nightly dependencies. Will let the build team comment on the best way to do this
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.
In other RAPIDS packages, these environment files are used to create the test environment. They do not go into built packages. I think the same is true for you here, but you know better than me.
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.
So, to the best of my understanding, my reasoning behind setting this to
rapidsai-nightlywas to make sure that the latest changes happening in thecuvslevel should be used, so that all required updates (especially the ones needed due to breaking changes incuvs) at thecuvs-lucenelevel are attended to in time (if not taken care of, already).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.
It's a mix. There's benefits and drawbacks to each method. With nightlies, you will catch new issues. You may spend more time fixing breaking changes in smaller chunks. You might update your software in ways that are incompatible with the previous release. This last issue is not a concern for us, because we do not support mixing different release versions of different packages.
I think your setting here is fine and safe. If you find the breaks come too often and you want to sort them out in larger batches, then remove it.
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 agree w/ @msarahan. I think this is fine.