Created a sklearn wrapper for the QA Pipeline

[andrelmfarias]

Created a QAPipeline class that works as a sklearn estimator to ease the process of question-answer

An instance of this class can be created by using a version of the Bert model with the sklearn wrapper. It will train a tf-idf matrix on the documents fed as a dataframe to the method QAPipeline().fit()

In order to obtain an answer to a question one only need to call the method QAPipeline().answer() on a question passed as string.

[fmikaelian]

Just made a quick review of this:

To understand clearly, you want to implement a sklearn pipeline that can use fit() to train both retriever and reader and predict() to output final answer directly. Did I understood correctly?

[andrelmfarias]

Not exactly.

This pipeline has two different methods for training: fit() and fit_bert()

The first one trains the retriever and the second one trains the reader (what we are rarely going to do, as we already have pre-trained models)

I also has a predict() method, which return the same results as the predict() method of the BertQA()class; and an answer() method, which only returns the answer to a question (what we are actually interested in).

[fmikaelian]

I see obvious benefits to the new class you built, among them a better usability and less complexity thanks to a single pipeline. Can you see other reasons why you implement this?

[andrelmfarias]

These are the only reasons I thought when I implemented it.

I still have other ideas for future improvements.

For example, instead of passing a dataframe with a such specific structure as we are doing now, we could create this dataframe inside the fit() method and pass a list of documents (articles) to it. The fit()method would then create the paragraphs and the dataframe.

I think the package will be cleaner and easier to use with this feature. We can discuss it further later on.

[fmikaelian]

Ok, very clear. I agree on the principle of this pipeline.

I would like to discuss more about some naming conventions and API details though. Let me review this in more details tomorrow and we can do a quick catch up call to discuss if needed.

Thank you for your work!

[fmikaelian]

Can you explain this?

[andrelmfarias]

If there is no pre-trained model fed as input when creating an instance of QAPipeline, it will create an instance of BertQA using the following parameters and will assign it to self.model attribute.

Please see line 124 of the code.

[fmikaelian]

For this one, let's talk over the phone 📞

[andrelmfarias]

OK.

I am available for a call tomorrow afternoon if that suits you.

[fmikaelian]

In my opinion, we should stick the sklearn API fit() and predict() methods only. Adding new API methods like fit_bert() or answer() to use our model is adding complexity to the user when we could have merged these methods easily.

[andrelmfarias]

I understand your point.

The problem with merging fit() and fit_bert() is that it will lead to a training of the BERT model each time we call fit(). We are not interested in this behavior as it will lead to needless computation time.

I agree with merging between predict() and answer(). However, I would prefer that a predict() method will only return the answer, instead of the 4 variables that are sent as a return as it is done with BertQA().predict().

[fmikaelian]

Let's make an empathy exercise for the end users:

The problem with merging fit() and fit_bert() is that it will lead to a training of the BERT model each time we call fit(). We are not interested in this behavior as it will lead to needless computation time.

As a basic user of cdqa and having access to pre-trained models, I would like to fit() the pipeline on my custom dataset and make predictions on it given my questions. Therefore the fit() method should not fit the reader but only the retriever since I don't need to train the base model of fine-tune it and I want to see results as quickly as possible.

As a advanced user of cdqa, I would like to fit() the pipeline on my custom dataset and make predictions on it given my questions with the best perfomance possible. I am confident that I can have better results with fine tuning the reader on my dataset and I have a GPU for that.

I therefore think that the fit method should allow for both retriever and reader training, at the choice of the user.

I agree with merging between predict() and answer(). However, I would prefer that a predict() method will only return the answer, instead of the 4 variables that are sent as a return as it is done with BertQA().predict().

I agree with you on this one. Should we do this in BertQA().predict() directly?

[andrelmfarias]

I therefore think that the fit method should allow for both retriever and reader training, at the choice of the user.

So you are proposing to merge both fit() functions and add a parameter to it, where the user will choose which model they will train? I agree with that

I agree with you on this one. Should we do this in BertQA().predict() directly?

Maybe. Honestly, I do not see any advantages in returning all_predictions, all_nbest_json, scores_diff_json together with final_prediction

[fmikaelian]

So you are proposing to merge both fit() functions and add a parameter to it, where the user will choose which model they will train? I agree with that

Yes. I actually don't know if setting params in fit() is a good sklearn practice but I think it's a way to solve this temporarily.

Maybe. Honestly, I do not see any advantages in returning all_predictions, all_nbest_json, scores_diff_json together with final_prediction

I actually implemented it this way for debugging purposes because I could not differientiate all of them, but now that the workflow is validated removing it makes sense 😅 I'll update bert_sklearn.py accordingly.

[fmikaelian]

Also want to highlight the fact that such a pipeline will impact the README.md documentation with the new (and more simple) workflow you introduced.

[fmikaelian]

@andrelmfarias

Please check my new sklearn class for the retriever:

https://github.com/fmikaelian/cdQA/blob/13bee06a0f5ef625d9b2f1922b1ee8ff875ce0bf/examples/tutorial-predict.ipynb

[andrelmfarias]

@fmikaelian

Thank you for this.

So now I can create an instance of this class inside the qa_pipeline and commit before the merge ?

[fmikaelian]

You can add your pipeline in the cdqa_sklearn.py script with the changes we discussed:

https://github.com/fmikaelian/cdQA/blob/develop/cdqa/pipeline/cdqa_sklearn.py

I am still not very clear about this #101 (comment)

But this step is the last one before we build a REST API to serve a demo UI ! I started implementing it there: https://github.com/fmikaelian/cdQA/blob/develop/api.py

[andrelmfarias]

So should I create a new branch in order to add the pipeline to the cdqa_sklearn.py script?

What about this branch?

[andrelmfarias]

@fmikaelian
I have implemented the changes as discussed. I tested the fit() method for the retriever and the predict() method in a notebook included in examples. Everything is working fine.

Could you please test the fit() method for the reader in GPU to check if everything is ok?

Please note that the current implementation of QAPipeline still uses the TfidfRetriever as you have implemented (passing the dataframe column as input to TfidfRetriever.fit(df['content'])). It should be changed once you will have implemented the improvement I proposed in #95.

[fmikaelian]

Thank you for sticking to that PR to make it viable. This is what a good data scientist should do.

Let's merge your work! 🙏