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

Add replication for MS MARCO passage experiment #25

Merged
merged 5 commits into from
May 22, 2020
Merged

Add replication for MS MARCO passage experiment #25

merged 5 commits into from
May 22, 2020

Conversation

MXueguang
Copy link
Member

Replication Status:
Success (No issues)

Environment:
OS: Ubuntu18.04 LTS
Python: 3.7.6
Pytorch: 1.5.0
GPU: Tesla P4
CUDA: 10.2

Replication Results:
monoBERT:
561590162471_ pic_hd
monoT5:
571590164481_ pic_hd

@lintool
Copy link
Member

lintool commented May 22, 2020

@ronakice do we really want separate lists for both models?

How about:

Results replicated by @MXueguang on 2020-05-22(commit 69de7db) (model 1 + model 2...)

@ronakice
Copy link
Member

ronakice commented May 22, 2020

@lintool in the case that we add more models (Which we probably will) I think this would be helpful? I figured that having people say (model 1 + model 2 + model 3) and occasionally just (model 2) is kinda messy? Unless we have separate docs for each.

@ronakice
Copy link
Member

@MXueguang great job being the first to replicate our results! Do you happen to remember the amount of time it took on the P4 (not the preprocessing just curious about the 'inference' itself, I see monoT5 took ~27 minutes!

@MXueguang
Copy link
Member Author

@ronakice
monoBERT took 1:00:10 (default batch size)
monoT5 took 27:36 (batch size = 96)

@lintool
Copy link
Member

lintool commented May 22, 2020

@lintool in the case that we add more models (Which we probably will) I think this would be helpful? I figured that having people say (model 1 + model 2 + model 3) and occasionally just (model 2) is kinda messy? Unless we have separate docs for each.

I think we should keep the models manageable per page? So, this being the "intro", no more models on this page. What do you think?

@ronakice
Copy link
Member

That sounds like a good idea. docs/experiments-msmarco-passage.md serves as an intro linking into say docs/experiments-msmarco-passage-monot5.md and docs/experiments-msmarco-passage-monobert.md

@lintool
Copy link
Member

lintool commented May 22, 2020

That sounds like a good idea. docs/experiments-msmarco-passage.md serves as an intro linking into say docs/experiments-msmarco-passage-monot5.md and docs/experiments-msmarco-passage-monobert.md

But I think two models on the same page is reasonable... so keep the page content as is (but fold the two replication sections into one); if we want to add another model, we create a new page?

@ronakice
Copy link
Member

Sounds like a plan. Yes, I was thinking that might be a good idea too since these are both similar enough pointwise estimators. Do we need to track GPUs this has worked on? We can assume that whoever replicates monoBERT can also replicate monoT5-base since it is quite a bit faster and ignore the (model 1 + model 2) part too. Something like

Results replicated by @MXueguang on 2020-05-22 (commit 69de7db) (Tesla P4)

@lintool
Copy link
Member

lintool commented May 22, 2020

Results replicated by @MXueguang on 2020-05-22 (commit 69de7db) (Tesla P4)

👍 on this. We can always track back to PR for more details.

@lintool
Copy link
Member

lintool commented May 22, 2020

@MXueguang please add the GPU, and @ronakice can merge.

@ronakice
Copy link
Member

@MXueguang you can also remove the (monoBERT + monoT5) part. LGTM other than that!

@ronakice
Copy link
Member

Awesome, great job @MXueguang

@ronakice ronakice merged commit 6e9dfc6 into castorini:master May 22, 2020
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.

3 participants