first version of save_to_remote for HF from FarmReader

[TuanaCelik]

Fixes #2416

This draft PR introduces the save_to_remote() function to the FarmReader
It converts a model to the transformer models format using the convert_to_transformer method from the AdaptiveModel class.
I have tested this by fine-tuning a model and then calling save_to_remote - then loading that same model back into FarmReader and using it in an ExtractiveQuestionAnswering pipeline

(PS: sentence-transformers implements this in their own repo and I was able to reuse their method of tracking files for lfs)

To do and test

• I am unsure how this would behave in the case the repo already exists on HF. The create_repo method from huggingface_hub has an exists_ok argument which I've set to True but this still needs testing
• The draft/default model card
• Testing that this structure of model is ok and can generalise to other types of models(?) that could be trained with FarmReader
• Once we're happy with this, we would need the same for the other two classes: DensePassageRetriever and TableTextRetriever

Status (please check what you already did):

• First draft (up for discussions & feedback)
• Final code
• Enable actions on your fork
• Added tests
• Updated documentation

[ZanSara]

@TuanaCelik I recently merged a drastic change to the CI, so I recommend you merge master before proceeding with this one. Thanks 😊

[tstadel]

@TuanaCelik do you have some test notebook / code to try this out?

[tstadel]

So usage would be:

from haystack.nodes import FARMReader

reader = FARMReader(model_name_or_path="distilbert-base-uncased-distilled-squad", use_gpu=True)
data_dir = "data"

reader.train(data_dir=data_dir, train_filename="squad_small.json", use_gpu=True, n_epochs=1, save_dir="my_model", batch_size=4)

reader.save_to_remote(model_name='model_from_haystack', private=True)

And reloading the model from HF

new_reader = FARMReader(model_name_or_path='HF_USER_NAME/model_from_haystack', use_auth_token=True)

[tstadel]

Looks already pretty good! Let's not save FARM-related stuff and add some docstrings. Then it's good to go for a first quick-win version.

[tstadel]

If the repo already exists it will overwrite the model there. So as a next iteration, adding a version parameter would be an option.

[TuanaCelik]

Thanks @tstadel - I'll add the docstrings and change the self.save as suggested. 2 things I would like to clarify:

1. For this PR shall we keep it condensed to adding this functionality to just FARMReader and leave the others (DensePassageRetriever and TableTextRetriever) for a second iteration?
2. You mentioned 'as a next iteration, adding a version parameter would be an option' -> again, in this PR or a following one?

In short, these could be smaller PRs and changes later. What would be best? Do all in one or merge this simple version with just FARMReader first (after some minor changes + docstrings)

[tstadel]

Thanks @tstadel - I'll add the docstrings and change the self.save as suggested. 2 things I would like to clarify:

1. For this PR shall we keep it condensed to adding this functionality to just FARMReader and leave the others (DensePassageRetriever and TableTextRetriever) for a second iteration?
2. You mentioned 'as a next iteration, adding a version parameter would be an option' -> again, in this PR or a following one?

In short, these could be smaller PRs and changes later. What would be best? Do all in one or merge this simple version with just FARMReader first (after some minor changes + docstrings)

I'd say let's do all of the extensions in separate PRs. This PR would already create value if we released it tomorrow.

[tstadel]

@TuanaCelik do you have time to work on this? If not I could take over from here...

[TuanaCelik]

@tstadel - working on it this week so can be left with me :)

[TuanaCelik]

@ZanSara @tstadel - I've made changes based on the comments. 2 things:
I've referenced sentence-transformers repo in the docstring but this can be moved into a comment.
I can imagine moving the git-lfs check and tracking section to utils to be re-used in the other save_to_remote'esque functions we'll have for the other nodes. Could be moved when needed and we're adding more of these functions

[tstadel]

Good that pylint spotted this during CI: we should avoid positional args when calling create_repo. Please switch to keyword args.

[tstadel]

Sorry to bother you again. I executed my test script again and I got the following warning:

FutureWarning: `name` and `organization` input arguments are deprecated and will be removed in v0.10. Pass `repo_id` instead.

Let's briefly discuss whether we want to use repo_id (would be e.g. "deepset/my_model_name") in the first place instead of model_name (would be "my_model_name") and hf_organization (would be "deepset") params. I think it would be easier to just have the repo_id param. We don't need to add any additional conversion logic when v0.10 is finally out.
@TuanaCelik what do you think?

[julian-risch]

Please also have a look at this comment on storing models in FARM format on the model hub and failing loading: https://github.com/deepset-ai/haystack-hub-api/issues/977#issuecomment-1173439438

[tstadel]

Please also have a look at this comment on storing models in FARM format on the model hub and failing loading: deepset-ai/haystack-hub-api#977 (comment)

@julian-risch we should be safe here as we first convert the model to transformers and then upload.

[TuanaCelik]

We changed the huggingface-hub version requirements in setup.cfg with @tstadel to acomodate both for audio and FARMReader's save_to_remote()

[tstadel]

LGTM!