You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "shubhamvishu (via GitHub)" <gi...@apache.org> on 2023/11/11 20:31:33 UTC
[PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
shubhamvishu opened a new pull request, #12799:
URL: https://github.com/apache/lucene/pull/12799
### Description
This PR proposes to :
- Make the `TaskExecutor` public which is currently pkg-private - As we are making more use to concurrency and not restrictive at searching time eg: now at indexing time we concurrently create the hnsw graph (#12660). We could use the `TaskExecutor` implementation to do this for us.
- Use `TaskExecutor#invokeAll` in `HnswConcurrentMergeBuilder#build` to run the workers concurrently.
<!--
If this is your first contribution to Lucene, please make sure you have reviewed the contribution guide.
https://github.com/apache/lucene/blob/main/CONTRIBUTING.md
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1400409812
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java:
##########
@@ -160,12 +160,12 @@ public Lucene99HnswVectorsFormat(int maxConn, int beamWidth) {
* @param maxConn the maximum number of connections to a node in the HNSW graph
* @param beamWidth the size of the queue maintained during graph construction.
* @param numMergeWorkers number of workers (threads) that will be used when doing merge. If
- * larger than 1, a non-null {@link ExecutorService} must be passed as mergeExec
- * @param mergeExec the {@link ExecutorService} that will be used by ALL vector writers that are
+ * larger than 1, a non-null {@link TaskExecutor} must be passed as mergeExec
+ * @param mergeExec the {@link TaskExecutor} that will be used by ALL vector writers that are
* generated by this format to do the merge
*/
public Lucene99HnswVectorsFormat(
- int maxConn, int beamWidth, int numMergeWorkers, ExecutorService mergeExec) {
+ int maxConn, int beamWidth, int numMergeWorkers, TaskExecutor mergeExec) {
Review Comment:
Sure I'll change it, but tbh I feel maybe it was better to keep the ES itself instead of storing the TE reference because ES is capable of doing other things as well that TE does not support yet eg: timeout support etc. I understand there is no use case as such currently in hnsw indexing side here but if maybe there could be some in future(?). Just wanted to make this point here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1399275973
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/ConcurrentHnswMerger.java:
##########
@@ -41,7 +42,7 @@ public ConcurrentHnswMerger(
ExecutorService exec,
int numWorker) {
super(fieldInfo, scorerSupplier, M, beamWidth);
- this.exec = exec;
+ this.taskExecutor = new TaskExecutor(exec);
Review Comment:
Makes sense to me, changed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1399670946
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsWriter.java:
##########
@@ -489,8 +490,9 @@ private void writeMeta(
private HnswGraphMerger createGraphMerger(
FieldInfo fieldInfo, RandomVectorScorerSupplier scorerSupplier) {
if (mergeExec != null) {
+ TaskExecutor taskExecutor = new TaskExecutor(mergeExec);
Review Comment:
Hmm....so I think the way would be to not have ExecutorService and let the caller pass the TE itself. For that we should give more up in the hierarchy to have a TE and not ExecutorService. i.e. `Lucene99HnswVectorsFormat`. Does that sound good?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1399656766
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsWriter.java:
##########
@@ -489,8 +490,9 @@ private void writeMeta(
private HnswGraphMerger createGraphMerger(
FieldInfo fieldInfo, RandomVectorScorerSupplier scorerSupplier) {
if (mergeExec != null) {
+ TaskExecutor taskExecutor = new TaskExecutor(mergeExec);
Review Comment:
Can't this class hold directly a TaskExecutor instead of an ExecutorService? The null check can be made against the task executor too? That way we would create a single task executor instance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1395692713
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -53,7 +53,7 @@ public final class TaskExecutor {
private final Executor executor;
- TaskExecutor(Executor executor) {
+ public TaskExecutor(Executor executor) {
Review Comment:
Ahh I see....just wondering why it wasn't caught locally with `gradlew check`. I think it should have? Also the initial revision passed successfully which had it public too. Dunno but maybe there is some opportunity for some sort of fix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on PR #12799:
URL: https://github.com/apache/lucene/pull/12799#issuecomment-1818960509
@javanna I have addressed the latest comments now. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1399047980
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java:
##########
@@ -77,42 +75,17 @@ public OnHeapHnswGraph build(int maxOrd) throws IOException {
HNSW_COMPONENT,
"build graph from " + maxOrd + " vectors, with " + workers.length + " workers");
}
- List<Future<?>> futures = new ArrayList<>();
+ List<Callable<Void>> futures = new ArrayList<>();
for (int i = 0; i < workers.length; i++) {
int finalI = i;
futures.add(
- exec.submit(
- () -> {
- try {
- workers[finalI].run(maxOrd);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }));
- }
- Throwable exc = null;
- for (Future<?> future : futures) {
- try {
- future.get();
- } catch (InterruptedException e) {
- var newException = new ThreadInterruptedException(e);
- if (exc == null) {
- exc = newException;
- } else {
- exc.addSuppressed(newException);
- }
- } catch (ExecutionException e) {
- if (exc == null) {
- exc = e.getCause();
- } else {
- exc.addSuppressed(e.getCause());
- }
- }
- }
- if (exc != null) {
- // The error handling was copied from TaskExecutor. should we just use TaskExecutor instead?
- throw IOUtils.rethrowAlways(exc);
+ () -> {
+ workers[finalI].run(maxOrd);
+ return null;
+ });
}
+ TaskExecutor taskExecutor = new TaskExecutor(exec);
Review Comment:
I see what you mean, I believe you are correct. Would it make sense though to carry around the TaskExecutor instance instead of the executor service, and create it earlier as soon as the executor is provided initially as a constructor argument?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1399120194
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java:
##########
@@ -77,42 +75,17 @@ public OnHeapHnswGraph build(int maxOrd) throws IOException {
HNSW_COMPONENT,
"build graph from " + maxOrd + " vectors, with " + workers.length + " workers");
}
- List<Future<?>> futures = new ArrayList<>();
+ List<Callable<Void>> futures = new ArrayList<>();
for (int i = 0; i < workers.length; i++) {
int finalI = i;
futures.add(
- exec.submit(
- () -> {
- try {
- workers[finalI].run(maxOrd);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }));
- }
- Throwable exc = null;
- for (Future<?> future : futures) {
- try {
- future.get();
- } catch (InterruptedException e) {
- var newException = new ThreadInterruptedException(e);
- if (exc == null) {
- exc = newException;
- } else {
- exc.addSuppressed(newException);
- }
- } catch (ExecutionException e) {
- if (exc == null) {
- exc = e.getCause();
- } else {
- exc.addSuppressed(e.getCause());
- }
- }
- }
- if (exc != null) {
- // The error handling was copied from TaskExecutor. should we just use TaskExecutor instead?
- throw IOUtils.rethrowAlways(exc);
+ () -> {
+ workers[finalI].run(maxOrd);
+ return null;
+ });
}
+ TaskExecutor taskExecutor = new TaskExecutor(exec);
Review Comment:
> Would it make sense though to carry around the TaskExecutor instance instead of the executor service, and create it earlier as soon as the executor is provided initially as a constructor argument?
Yes, I would prefer it that way(its also similar to `IndexSearcher`). I have that change ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on PR #12799:
URL: https://github.com/apache/lucene/pull/12799#issuecomment-1814218891
@javanna I have added the CHANGES entry and addressed the comment. Seems the precommit fails on to `:lucene:documentation:markdownToHtml` task which looks unrelated? Not sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna merged PR #12799:
URL: https://github.com/apache/lucene/pull/12799
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on PR #12799:
URL: https://github.com/apache/lucene/pull/12799#issuecomment-1819737107
> I left one last small comment, sorry I could have made my earlier comment clearer and avoid more back and forth. Thanks for your patience.
No problem. I have addressed your comment to hold the TE in the classes instead of ES in the latest revision. Thanks for reviewing @javanna !
> Could you also address the merge conflict in the changes file please?
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1396959558
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java:
##########
@@ -77,42 +75,17 @@ public OnHeapHnswGraph build(int maxOrd) throws IOException {
HNSW_COMPONENT,
"build graph from " + maxOrd + " vectors, with " + workers.length + " workers");
}
- List<Future<?>> futures = new ArrayList<>();
+ List<Callable<Void>> futures = new ArrayList<>();
for (int i = 0; i < workers.length; i++) {
int finalI = i;
futures.add(
- exec.submit(
- () -> {
- try {
- workers[finalI].run(maxOrd);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }));
- }
- Throwable exc = null;
- for (Future<?> future : futures) {
- try {
- future.get();
- } catch (InterruptedException e) {
- var newException = new ThreadInterruptedException(e);
- if (exc == null) {
- exc = newException;
- } else {
- exc.addSuppressed(newException);
- }
- } catch (ExecutionException e) {
- if (exc == null) {
- exc = e.getCause();
- } else {
- exc.addSuppressed(e.getCause());
- }
- }
- }
- if (exc != null) {
- // The error handling was copied from TaskExecutor. should we just use TaskExecutor instead?
- throw IOUtils.rethrowAlways(exc);
+ () -> {
+ workers[finalI].run(maxOrd);
+ return null;
+ });
}
+ TaskExecutor taskExecutor = new TaskExecutor(exec);
Review Comment:
I think with this we are only creating 1 task executor instance per knn field because the concurrent graph building happens in `HnswConcurrentMergeBuilder#build`(where we make use of concurrency) so moving the task executor creation above in the hierarchy(to `ConcurrentHnswMerger`) shouldn't make any difference as we would still be creating 1 instance per knn field or maybe I'm wrong?
I'm happy to change it if this is not the case or if you feel thats still better approach to pass the task executor from above instead of executor service?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1396959558
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java:
##########
@@ -77,42 +75,17 @@ public OnHeapHnswGraph build(int maxOrd) throws IOException {
HNSW_COMPONENT,
"build graph from " + maxOrd + " vectors, with " + workers.length + " workers");
}
- List<Future<?>> futures = new ArrayList<>();
+ List<Callable<Void>> futures = new ArrayList<>();
for (int i = 0; i < workers.length; i++) {
int finalI = i;
futures.add(
- exec.submit(
- () -> {
- try {
- workers[finalI].run(maxOrd);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }));
- }
- Throwable exc = null;
- for (Future<?> future : futures) {
- try {
- future.get();
- } catch (InterruptedException e) {
- var newException = new ThreadInterruptedException(e);
- if (exc == null) {
- exc = newException;
- } else {
- exc.addSuppressed(newException);
- }
- } catch (ExecutionException e) {
- if (exc == null) {
- exc = e.getCause();
- } else {
- exc.addSuppressed(e.getCause());
- }
- }
- }
- if (exc != null) {
- // The error handling was copied from TaskExecutor. should we just use TaskExecutor instead?
- throw IOUtils.rethrowAlways(exc);
+ () -> {
+ workers[finalI].run(maxOrd);
+ return null;
+ });
}
+ TaskExecutor taskExecutor = new TaskExecutor(exec);
Review Comment:
I think with this we are only creating 1 task executor instance per knn field because the concurrent graph building happens in `HnswConcurrentMergeBuilder#build` so moving the task executor creation above in the hierarchy(to `ConcurrentHnswMerger`) shouldn't make any difference as we would still be creating 1 instance per knn field or maybe I'm wrong?
I'm happy to change it if this is not the case or if you feel thats still better approach to pass the task executor from above instead of executor service?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1395505763
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java:
##########
@@ -77,42 +75,21 @@ public OnHeapHnswGraph build(int maxOrd) throws IOException {
HNSW_COMPONENT,
"build graph from " + maxOrd + " vectors, with " + workers.length + " workers");
}
- List<Future<?>> futures = new ArrayList<>();
+ List<Callable<Void>> futures = new ArrayList<>();
for (int i = 0; i < workers.length; i++) {
int finalI = i;
futures.add(
- exec.submit(
- () -> {
- try {
- workers[finalI].run(maxOrd);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }));
- }
- Throwable exc = null;
- for (Future<?> future : futures) {
- try {
- future.get();
- } catch (InterruptedException e) {
- var newException = new ThreadInterruptedException(e);
- if (exc == null) {
- exc = newException;
- } else {
- exc.addSuppressed(newException);
- }
- } catch (ExecutionException e) {
- if (exc == null) {
- exc = e.getCause();
- } else {
- exc.addSuppressed(e.getCause());
- }
- }
- }
- if (exc != null) {
- // The error handling was copied from TaskExecutor. should we just use TaskExecutor instead?
- throw IOUtils.rethrowAlways(exc);
+ () -> {
+ try {
+ workers[finalI].run(maxOrd);
+ return null;
+ } catch (IOException e) {
Review Comment:
I don't think so. I retained this as it is. I'll remove it in the next revision
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1400536223
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java:
##########
@@ -160,12 +160,12 @@ public Lucene99HnswVectorsFormat(int maxConn, int beamWidth) {
* @param maxConn the maximum number of connections to a node in the HNSW graph
* @param beamWidth the size of the queue maintained during graph construction.
* @param numMergeWorkers number of workers (threads) that will be used when doing merge. If
- * larger than 1, a non-null {@link ExecutorService} must be passed as mergeExec
- * @param mergeExec the {@link ExecutorService} that will be used by ALL vector writers that are
+ * larger than 1, a non-null {@link TaskExecutor} must be passed as mergeExec
+ * @param mergeExec the {@link TaskExecutor} that will be used by ALL vector writers that are
* generated by this format to do the merge
*/
public Lucene99HnswVectorsFormat(
- int maxConn, int beamWidth, int numMergeWorkers, ExecutorService mergeExec) {
+ int maxConn, int beamWidth, int numMergeWorkers, TaskExecutor mergeExec) {
Review Comment:
Thanks for voicing your opinion. My main goal was to create one instance of TaskExecutor per Executor, hence create it as soon as possible. I also believe that we will want to extend TaskExecutor's capabilities instead of relying on executor service capabilities in specific places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1395692713
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -53,7 +53,7 @@ public final class TaskExecutor {
private final Executor executor;
- TaskExecutor(Executor executor) {
+ public TaskExecutor(Executor executor) {
Review Comment:
Ahh I see....just wondering why it wasn't caught locally with `gradlew check`. I think it should have?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1395689207
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -53,7 +53,7 @@ public final class TaskExecutor {
private final Executor executor;
- TaskExecutor(Executor executor) {
+ public TaskExecutor(Executor executor) {
Review Comment:
Now that its public, you might have to add a JavaDoc here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1396067925
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java:
##########
@@ -77,42 +75,17 @@ public OnHeapHnswGraph build(int maxOrd) throws IOException {
HNSW_COMPONENT,
"build graph from " + maxOrd + " vectors, with " + workers.length + " workers");
}
- List<Future<?>> futures = new ArrayList<>();
+ List<Callable<Void>> futures = new ArrayList<>();
for (int i = 0; i < workers.length; i++) {
int finalI = i;
futures.add(
- exec.submit(
- () -> {
- try {
- workers[finalI].run(maxOrd);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }));
- }
- Throwable exc = null;
- for (Future<?> future : futures) {
- try {
- future.get();
- } catch (InterruptedException e) {
- var newException = new ThreadInterruptedException(e);
- if (exc == null) {
- exc = newException;
- } else {
- exc.addSuppressed(newException);
- }
- } catch (ExecutionException e) {
- if (exc == null) {
- exc = e.getCause();
- } else {
- exc.addSuppressed(e.getCause());
- }
- }
- }
- if (exc != null) {
- // The error handling was copied from TaskExecutor. should we just use TaskExecutor instead?
- throw IOUtils.rethrowAlways(exc);
+ () -> {
+ workers[finalI].run(maxOrd);
+ return null;
+ });
}
+ TaskExecutor taskExecutor = new TaskExecutor(exec);
Review Comment:
Would it make sense to not create a new instance all the time, given that the provided executor is final? Maybe we could create the TaskExecutor and carry it around already in ConcurrentHnswMerger?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1395446929
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java:
##########
@@ -77,42 +75,21 @@ public OnHeapHnswGraph build(int maxOrd) throws IOException {
HNSW_COMPONENT,
"build graph from " + maxOrd + " vectors, with " + workers.length + " workers");
}
- List<Future<?>> futures = new ArrayList<>();
+ List<Callable<Void>> futures = new ArrayList<>();
for (int i = 0; i < workers.length; i++) {
int finalI = i;
futures.add(
- exec.submit(
- () -> {
- try {
- workers[finalI].run(maxOrd);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }));
- }
- Throwable exc = null;
- for (Future<?> future : futures) {
- try {
- future.get();
- } catch (InterruptedException e) {
- var newException = new ThreadInterruptedException(e);
- if (exc == null) {
- exc = newException;
- } else {
- exc.addSuppressed(newException);
- }
- } catch (ExecutionException e) {
- if (exc == null) {
- exc = e.getCause();
- } else {
- exc.addSuppressed(e.getCause());
- }
- }
- }
- if (exc != null) {
- // The error handling was copied from TaskExecutor. should we just use TaskExecutor instead?
- throw IOUtils.rethrowAlways(exc);
+ () -> {
+ try {
+ workers[finalI].run(maxOrd);
+ return null;
+ } catch (IOException e) {
Review Comment:
Is it necessary to catch IOException here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #12799:
URL: https://github.com/apache/lucene/pull/12799#issuecomment-1821249014
Thanks @shubhamvishu !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1400409812
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java:
##########
@@ -160,12 +160,12 @@ public Lucene99HnswVectorsFormat(int maxConn, int beamWidth) {
* @param maxConn the maximum number of connections to a node in the HNSW graph
* @param beamWidth the size of the queue maintained during graph construction.
* @param numMergeWorkers number of workers (threads) that will be used when doing merge. If
- * larger than 1, a non-null {@link ExecutorService} must be passed as mergeExec
- * @param mergeExec the {@link ExecutorService} that will be used by ALL vector writers that are
+ * larger than 1, a non-null {@link TaskExecutor} must be passed as mergeExec
+ * @param mergeExec the {@link TaskExecutor} that will be used by ALL vector writers that are
* generated by this format to do the merge
*/
public Lucene99HnswVectorsFormat(
- int maxConn, int beamWidth, int numMergeWorkers, ExecutorService mergeExec) {
+ int maxConn, int beamWidth, int numMergeWorkers, TaskExecutor mergeExec) {
Review Comment:
Sure I'll change it, but tbh I feel maybe it was better to keep the ES itself instead of storing the TE reference because ES is capable of doing other things as well that TE does not support yet eg: timeout support etc. I understand there is no use case as such currently in hnsw indexing side here but if maybe there could be some in future that would help in having this a bit extensible(?). Just wanted to make this point here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1400609546
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java:
##########
@@ -160,12 +160,12 @@ public Lucene99HnswVectorsFormat(int maxConn, int beamWidth) {
* @param maxConn the maximum number of connections to a node in the HNSW graph
* @param beamWidth the size of the queue maintained during graph construction.
* @param numMergeWorkers number of workers (threads) that will be used when doing merge. If
- * larger than 1, a non-null {@link ExecutorService} must be passed as mergeExec
- * @param mergeExec the {@link ExecutorService} that will be used by ALL vector writers that are
+ * larger than 1, a non-null {@link TaskExecutor} must be passed as mergeExec
+ * @param mergeExec the {@link TaskExecutor} that will be used by ALL vector writers that are
* generated by this format to do the merge
*/
public Lucene99HnswVectorsFormat(
- int maxConn, int beamWidth, int numMergeWorkers, ExecutorService mergeExec) {
+ int maxConn, int beamWidth, int numMergeWorkers, TaskExecutor mergeExec) {
Review Comment:
@javanna Totally makes sense to me! I have addressed your comment in the latest commit. Please have a look. Thanks!
> The idea would be that we use TaskExecutor for general task execution, and that is used in different places like query rewrite, search against slices, loading of terms statistics, and now also building the graph.
+1 to extend TE capabilities and use it for all concurrency use cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1400536223
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java:
##########
@@ -160,12 +160,12 @@ public Lucene99HnswVectorsFormat(int maxConn, int beamWidth) {
* @param maxConn the maximum number of connections to a node in the HNSW graph
* @param beamWidth the size of the queue maintained during graph construction.
* @param numMergeWorkers number of workers (threads) that will be used when doing merge. If
- * larger than 1, a non-null {@link ExecutorService} must be passed as mergeExec
- * @param mergeExec the {@link ExecutorService} that will be used by ALL vector writers that are
+ * larger than 1, a non-null {@link TaskExecutor} must be passed as mergeExec
+ * @param mergeExec the {@link TaskExecutor} that will be used by ALL vector writers that are
* generated by this format to do the merge
*/
public Lucene99HnswVectorsFormat(
- int maxConn, int beamWidth, int numMergeWorkers, ExecutorService mergeExec) {
+ int maxConn, int beamWidth, int numMergeWorkers, TaskExecutor mergeExec) {
Review Comment:
Thanks for voicing your opinion. My main goal was to create one instance of TaskExecutor per Executor, hence create it as soon as possible. I also believe that we will want to extend TaskExecutor's capabilities instead of relying on executor service capabilities in specific places. The idea would be that we use TaskExecutor for general task execution, and that is used in different places like query rewrite, search against slices, loading of terms statistics, and now also building the graph.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1399178223
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/ConcurrentHnswMerger.java:
##########
@@ -41,7 +42,7 @@ public ConcurrentHnswMerger(
ExecutorService exec,
int numWorker) {
super(fieldInfo, scorerSupplier, M, beamWidth);
- this.exec = exec;
+ this.taskExecutor = new TaskExecutor(exec);
Review Comment:
Sorry for being picky :) this is good, but I think we can take a TaskExecutor here that gets created by the caller? Does that make sense?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12799:
URL: https://github.com/apache/lucene/pull/12799#discussion_r1400378967
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java:
##########
@@ -160,12 +160,12 @@ public Lucene99HnswVectorsFormat(int maxConn, int beamWidth) {
* @param maxConn the maximum number of connections to a node in the HNSW graph
* @param beamWidth the size of the queue maintained during graph construction.
* @param numMergeWorkers number of workers (threads) that will be used when doing merge. If
- * larger than 1, a non-null {@link ExecutorService} must be passed as mergeExec
- * @param mergeExec the {@link ExecutorService} that will be used by ALL vector writers that are
+ * larger than 1, a non-null {@link TaskExecutor} must be passed as mergeExec
+ * @param mergeExec the {@link TaskExecutor} that will be used by ALL vector writers that are
* generated by this format to do the merge
*/
public Lucene99HnswVectorsFormat(
- int maxConn, int beamWidth, int numMergeWorkers, ExecutorService mergeExec) {
+ int maxConn, int beamWidth, int numMergeWorkers, TaskExecutor mergeExec) {
Review Comment:
I would have rather left ExecutorService as a constructor argument, but create a TaskExecutor out of it and hold a reference to it instead of the executor. Does that make sense to you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org