You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "alessandrobenedetti (via GitHub)" <gi...@apache.org> on 2024/03/19 16:08:17 UTC
[PR] Fix/solr 17018 branch 9 1 [solr]
alessandrobenedetti opened a new pull request, #2358:
URL: https://github.com/apache/solr/pull/2358
https://issues.apache.org/jira/browse/SOLR-17018
# Description
Learning To Rank was ignoring any queryLimit parameter (such as timeAllowed) during the reranking phase.
This means that a particularly expensive feature to extract may cause extremely slow responses and failures out of control.
# Solution
The idea is to check the query limits after the rescore of each search result (Learning To Rank rescore topK results).
If the query limit is exhausted before the completion of the rescoring, the reranking is aborted and reverted (all or nothing approach).
The response is marked as a partialResult and the original score and ranking are returned.
# Tests
A test has been added with a simple Learning To Rank model that uses just one single slow feature (implemented with a sleep function query).
This feature simulates a slow feature extraction.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix/solr 17018 branch 9 1 [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2358:
URL: https://github.com/apache/solr/pull/2358#issuecomment-2049611339
I glossed over this while simultaneously glossing over the branch_9x commit to see that you when to the same source files and made similar-ish looking changes. You did. In the branch_9x test I see `ltr_expensiveFeatureRescoringAndPartialResultsNotTolerated_shouldRaiseException` but you didn't port that test specifically (albeit did the others). Why not?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix/solr 17018 branch 9 1 [solr]
Posted by "alessandrobenedetti (via GitHub)" <gi...@apache.org>.
alessandrobenedetti merged PR #2358:
URL: https://github.com/apache/solr/pull/2358
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix/solr 17018 branch 9 1 [solr]
Posted by "alessandrobenedetti (via GitHub)" <gi...@apache.org>.
alessandrobenedetti commented on PR #2358:
URL: https://github.com/apache/solr/pull/2358#issuecomment-2049618343
> I glossed over this while simultaneously glossing over the branch_9x commit to see that you when to the same source files and made similar-ish looking changes. You did. In the branch_9x test I see `ltr_expensiveFeatureRescoringAndPartialResultsNotTolerated_shouldRaiseException` but you didn't port that test specifically (albeit did the others). Why not?
it seemed to me that the possibility of not tolerating partial results via a dedicated parameter was introduced later on (as part of the query limit work). As far as I verified it was not possible in 9.1?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix/solr 17018 branch 9 1 [solr]
Posted by "alessandrobenedetti (via GitHub)" <gi...@apache.org>.
alessandrobenedetti commented on PR #2358:
URL: https://github.com/apache/solr/pull/2358#issuecomment-2049310875
Adding @dsmiley as a reviewer as I noticed some work in the same area back then:
https://github.com/apache/lucene-solr/pull/1726/files
The rationale behind this additional porting is the fact that a client of mine agreed to sponsor this contribution (already merged in 10.0 and 9.6: https://github.com/apache/solr/pull/2348) only if also backported to 9.1.x (I mentioned that a few weeks ago in the Solr Slack).
The idea is to follow up once merged in 9.1.x with 9.1.2 release including this bugfix.
In terms of the contribution, this porting required a bit of additional time as the mechanism for query limits is massively different from 9.x and 10.x, specifically:
1) I originally thought I could leverage the Lucene TimeLimitingCollector (and related TimeExceededException). But after some investigations and debugging I realised this is only valid for a first-stage retrieval and scoring, and it's not usable in the reranking phase easily.
2) Then I thought of using the ExitableDirectoryReader.ExitingReaderException mechanism, but following this approach, the solr document result list is emptied here:
org/apache/solr/handler/component/SearchHandler.java:440 when returning partial results.
Changing those parts to align with the expectations here, looked invasive and didn't like the outcome
So I ended up with this solution, building on top of the SolrQueryTimeoutImpl from @dsmiley past contribution.
It's decently clean and allowed me to mimic the same approach we have in 10.x and 9.x.
Happy to gather some feedback, and unless any concern I'll proceed in a week to the merge on 9.1.x.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org