Skip to content

Removed single_model_path; added infer_tokenizer to dpr load()#1060

Merged
julian-risch merged 1 commit intomasterfrom
dpr_without_bert_tokenizer
Jun 14, 2021
Merged

Removed single_model_path; added infer_tokenizer to dpr load()#1060
julian-risch merged 1 commit intomasterfrom
dpr_without_bert_tokenizer

Conversation

@julian-risch
Copy link
Copy Markdown
Member

@julian-risch julian-risch commented May 14, 2021 •
edited
Loading

Removed single_model_path because we don't use it anymore.
Added parameter infer_tokenizer_classes to load function of DensePassageRetriever so that DPR models with a non-BERT tokenizer can be loaded as required in #1046
The loading can be done as follows after initializing from model hub and saving to disk:

retriever = DensePassageRetriever(
    document_store=InMemoryDocumentStore(),
    query_embedding_model="etalab-ia/dpr-question_encoder-fr_qa-camembert",
    passage_embedding_model="etalab-ia/dpr-ctx_encoder-fr_qa-camembert",
    max_seq_len_query=64,
    max_seq_len_passage=256,
    infer_tokenizer_classes=True
)

save_dir = "../saved_models/dpr"
retriever.save(save_dir=save_dir)

loaded_retriever = DensePassageRetriever.load(load_dir=save_dir, document_store=None, infer_tokenizer_classes=True)

This PR corresponds to FARM PR #765

@julian-risch julian-risch requested a review from Timoeller May 14, 2021 15:50
@julian-risch julian-risch mentioned this pull request May 14, 2021
Merged
@julian-risch julian-risch changed the title WIP: Removed single_model_path; added infer_tokenizer to dpr load() Removed single_model_path; added infer_tokenizer to dpr load() Jun 2, 2021
@julian-risch julian-risch marked this pull request as ready for review June 2, 2021 19:27
tholor
tholor approved these changes Jun 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@tholor tholor left a comment

Choose a reason for hiding this comment

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

LGTM

Timoeller
Timoeller reviewed Jun 3, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@Timoeller Timoeller left a comment

Choose a reason for hiding this comment

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

The changes are good and solid.

Lets wait with merging until we have the new FARM version in Haystack so we can make use of the infer_tokenizer_class parameter when saving + loading.

@julian-risch
Copy link
Copy Markdown
Member Author

julian-risch commented Jun 14, 2021

We have the new FARM version in Haystack, so I am merging now.

@julian-risch julian-risch merged commit f6e70f0 into master Jun 14, 2021
@julian-risch julian-risch deleted the dpr_without_bert_tokenizer branch June 14, 2021 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tholor tholor tholor approved these changes

+1 more reviewer

@Timoeller Timoeller Timoeller left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@julian-risch @tholor @Timoeller