Thanks for the effort!

Following up on [this comment] (#1768 (comment)) from @KennethEnevoldsen, it's not quite clear in this PR why this new wrapper is needed, and why we cannot support prompts in the existing ST wrapper. If there was an existing discussion, it would be great to link here.

Could we please add an explanation clearly with code examples in the PR description (since there's no issue reference), and also a short line in the docstrings?

@isaac-chung This wrapper is designed for models that use prompts to generate embeddings. Previously, this was handled by InstructWrapper, but it relied on gritlm, which can be a bit difficult for newcomers to understand. A similar wrapper was used with the Jasper model, which was initially included in this PR. However, I removed it because it used s2p to decide whether to apply a passage prompt or not, and I wanted to keep this PR more reproducible.

I didn't add this functionality to SentenceTransformerWrapper because I believe it’s easier to have a separate wrapper specifically for models using instructions and prefixes. I'm not sure what kind of code example you're looking for, as this wrapper works similarly to all other wrappers.

Thanks for the explanation and the effort you put into this! I think this is a good step forward, but I have a few follow-up questions to help clarify things further:

1. Could we provide more guidance on how users should choose between the gritlm-instruct wrapper and the new instruct wrapper? Having two separate wrappers might introduce some confusion, so it would help to clarify their use cases. For example, are there any user feedback or specific scenarios that prompted this change?

2. I noticed there is only one file changed in this PR. Could you elaborate on what you mean by 'reproducible' here? A bit more context would help me understand how this is addressed.

3. Lastly, could you share more details on why this approach (a separate wrapper) is easier or preferable compared to extending the existing SentenceTransformerWrapper? If there are challenges in doing so, it would be helpful to document them for context.

Firstly, I made a PR to add the InstructSentenceTransformerWrapper as a replacement for the JasperWrapper. However, during the review, this turned into a discussion.

1. I believe we can deprecate (or just don't apply to new models) the GritLM and primarily use the SentenceTransformer. The GritLM is difficult to understand because it’s unclear what certain parameters mean. For example, it’s not obvious what [attn = "cccc"] represents. Additionally, GritLM uses AutoModel for loading models, but now there are models (like Jasper) that rely on custom classes automatically supported by SentenceTransformer. These cannot be run with GritLMWrapper.

2. The authors of Jasper apply instructions based on whether the task involves passages or not and also check if the task is an s2s task (source).

3. Combining wrappers that handle both prefixes and instructions could be confusing, as they involve different logic (e.g., applying prompts, adding special tokens, etc.). Keeping them separate would make it easier to maintain and avoid mixing their distinct behaviors.

I think we shouldn’t add this to the README because I don’t believe wrappers should be included there. Instead, it’s better to add it to docs/adding_a_model, as this change is more developer-focused than user-focused.

Good to merge 🙌