[codex] Add local metadata for registered topics - #3348
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 968dceaadf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3348 +/- ##
============================================
- Coverage 73.40% 72.25% -1.15%
- Complexity 1913 1918 +5
============================================
Files 206 206
Lines 13493 13013 -480
Branches 1867 1874 +7
============================================
- Hits 9904 9403 -501
- Misses 2843 2854 +11
- Partials 746 756 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Initial refactor pass by Codex. Will now de-slopify. |
|
@codex review |
|
Wrapping up reproductions, working fine - ready for review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f7c66e186
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lilyjge
left a comment
There was a problem hiding this comment.
would it make more sense for src/main/resources/topics-and-qrels/_local_metadata_topics.json and src/main/resources/topics-and-qrels/_local_metadata_topics_aliases.json to go in the tools repo? instead of an explicit topic reader class it could be a more vague enum topic type that get's mapped to the appropriate reader class in anserini/pyserini. the path and aliases could definitely be reused across pyserini/anserini as well
|
hi @lilyjge thanks for your review - responses:
Yupp, the plan is for the metadata files to go in https://github.com/castorini/anserini-tools/tree/master/topics-and-qrels eventually. I'm currently using This is to avoid churn, having to repeated touch files in another repo.
My current plan is to decouple Anserini/Pyserini and have both read the metadata separately into their own internal classes. This decoupling allows Anserini/Pyserini to advance without inter-dependencies. That is, if Pyserini depends on Anserini enums, then moving metadata to files doesn't shorten the dependency chain... you'd still have to modify Anserini, publish fatjar, and then modify Pyserini. Under the current plan I'm in the process of implementing, we would just modify the json metadata, and Anserini/Pyserini can read the new info (prebuilt indexes, topics, and qrels) independently. The downside here is that there will be redundant code reading the metadata (in Java and Python), but I think this is a worthwhile tradeoff (to decouple Pyserini from Anserini and shorten the dependency chain). Happy to discuss... |
|
this resolves my primary concern: yes, I agree with this! what I meant about an enum type for the queries is that instead of specifying the anserini reader class, to abstract a level higher so it can be shared across anserini/pyserini, eg design wise I think this is fine! I will take a pass through the code then |
Ah, I see. However, currently, pyserini needs Anserini TopicReaders to read the topics: https://github.com/castorini/pyserini/blob/master/pyserini/search/_base.py#L122 So we need to rethink a bit more... Yes, let's iterate on this further on another PR? |
| lookup.put(name, topic); | ||
| } | ||
|
|
||
| public static <K> SortedMap<K, Map<String, String>> resolve(String[] topicsArray, String topicReader) { |
There was a problem hiding this comment.
why did we remove this helper method? it was called in multiple places that now have chunks of code
| } else { | ||
| topics.putAll(TopicReader.getTopics(ref)); | ||
| } | ||
| Path topicsPath = Paths.get(topicsFile); |
There was a problem hiding this comment.
for future note, the fact that this section has basically the same diff as src/main/java/io/anserini/search/SearchFlatDenseVectors.java makes me think there can be more abstraction to reduce duplication
| @@ -813,47 +795,7 @@ public void testTREC20DL() throws IOException { | |||
| assertEquals("why did the ancient egyptians call their land ke ##met , or black land ?", topics.get(topics.lastKey()).get("title")); | |||
| assertEquals("who is aziz hash ##im", topics.get(1030303).get("title")); | |||
|
|
|||
| topics = TopicReader.getTopics(Topics.TREC2020_DL_UNICOIL); | |||
There was a problem hiding this comment.
why did these tests get removed instead of replaced? similarly below
Summary
Topicsinto a metadata-backed registry loaded from JSON resources.Topics.resolve(...)with explicitTopics.load(...)andTopicReader.load(...)entry points.TopicReader.load(file, topicReader), while registry keys useTopics.load(key).Validation
mvn -Dtest=TopicsTest,TopicReaderTest,TopicsRegistryTest testmvn -Dtest=SearchCollectionTest testgit diff --check