Skip to content
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

Feature/msmarco psg #117

Merged
merged 138 commits into from
Aug 26, 2021
Merged

Feature/msmarco psg #117

merged 138 commits into from
Aug 26, 2021

Conversation

crystina-z
Copy link
Collaborator

@crystina-z crystina-z commented Nov 21, 2020

Just sending this PR to better track the progress :) Don't worry about it now

Now running ms marco psg while only reranking the top100 data looks right.

Confusing stuff to solve

  • So far none of the runs on reranking top1k could save the checkpoint even tho it seems finished in some cases (but it can be saved in reranking the top100 case). While i was suspecting this is due to the running time limit in slurm, yet everytime queueing up a 4 days runs some other weird bug can pop up so after so many days it is still unconfirmed...
  • The most recent run throws this error, while the sampler.generate_example and sampler.get_preds_in_trec_format seem to align with each other, and the dev records are prepared in this run so it's not because of overdue cache data. Still checking what's happening here. (Again this does not happen for the reranking top100 case.)
Traceback (most recent call last):
  File "run.py", line 108, in <module>
    task_entry_function()
  File "/home/czhang/aaai2021/tmp/capreolus/capreolus/task/rerank.py", line 54, in train
    return self.rerank_run(best_search_run, self.get_results_path())
  File "/home/czhang/aaai2021/tmp/capreolus/capreolus/task/rerank.py", line 101, in rerank_run
    self.benchmark.relevance_level,
  File "/home/czhang/aaai2021/tmp/capreolus/capreolus/trainer/tensorflow.py", line 205, in train
    trec_preds = self.get_preds_in_trec_format(dev_predictions, dev_data)
  File "/home/czhang/aaai2021/tmp/capreolus/capreolus/trainer/tensorflow.py", line 429, in get_preds_in_trec_format
    pred_dict[qid][docid] = predictions[i].numpy().astype(np.float16).item()
IndexError: list index out of range

Features/Support to add

  • right now I have to hack the code to let the ranker only do evaluation on dev qids, otherwise the evaluation there (on millions of training queries!) gonna block the process forever. Probably this is already solved in feature/fit branch where only test qids are evaluated?
  • use sqlite to prepare and store the docid2passage, so this can be run on the RAM-poor machines
  • (maybe less urgent) handle the msmarco downloading using the allenai/ir_datasets

Sidenote (about the running time of some operation)

  1. evaluation (on only dev set, not including the training queries)
    1.1 pytrec_eval: 40 sec
    1.2 (trec_eval: 4 secs)
  2. Loading the whole runfile (including training data): ~310s (Saercher.load_trec_run())
  3. Preparing the passage: ?
  4. Prepare training runs: 3.5 hours
  5. tfrecords (train + dev): 2 hours
  6. training:
    6.1 (3k iteration) < 2 hours
    6.2 (30k iter) 11~12 hours
  7. inference
    7.1 (top100): several hours
    7.2 (top1k) > 1.5 days

@lgtm-com
Copy link

lgtm-com bot commented Nov 21, 2020

This pull request introduces 3 alerts when merging 649804e into bf50423 - view on LGTM.com

new alerts:

  • 3 for Unused import

Crystina333 and others added 22 commits November 25, 2020 14:48
… evaluator and msmarco benchmark accordingly. so that benchmark can specify by itself if they wana include train_qids in dev set for non-neural-net algorithms, where no training is needed
…s in qrels should be considered

(2) update all the places that called eval_runs and _eval_runs, ensuring the inputed qrels are filtered
(3) change the api of trainer.train, removing qrels and relevance_level, instead, sending an eval_fn(runs) to the train() which handles the evaluation logit completely
@lgtm-com
Copy link

lgtm-com bot commented Aug 8, 2021

This pull request introduces 2 alerts when merging 7664df9 into 18e31b7 - view on LGTM.com

new alerts:

  • 2 for Unused import

@crystina-z crystina-z changed the title [WIP] Feature/msmarco psg Feature/msmarco psg Aug 22, 2021
@lgtm-com
Copy link

lgtm-com bot commented Aug 22, 2021

This pull request introduces 1 alert when merging 569276e into 1767d5a - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Aug 22, 2021

This pull request introduces 1 alert when merging 1bbf0f2 into 1767d5a - view on LGTM.com

new alerts:

  • 1 for Unused import

Copy link
Member

@andrewyates andrewyates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! I left some minor questions/comments

capreolus/searcher/special.py Outdated Show resolved Hide resolved
capreolus/searcher/special.py Show resolved Hide resolved
capreolus/searcher/special.py Show resolved Hide resolved
capreolus/evaluator.py Show resolved Hide resolved
Creation Date : 06/12/2018
Last Modified : 1/21/2019
Authors : Daniel Campos <dacamp@microsoft.com>, Rutger van Haasteren <ruvanh@microsoft.com>
"""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually do we just keep the headings as it is?

@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2021

This pull request introduces 1 alert when merging bbe134e into 1767d5a - view on LGTM.com

new alerts:

  • 1 for Unused import

@andrewyates andrewyates merged commit 3521171 into master Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants