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

New tutorial: 19_multimodal_retrieval #52

Merged
merged 26 commits into from
Nov 7, 2022
Merged

Conversation

mayankjobanputra
Copy link
Contributor

A tutorial about the MultiModalRetriever, and built a simple retrieval pipeline that searches relevant images given a text query.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@TuanaCelik
Copy link
Member

We don't have automatic preview set up yet for tutorials in the new website but I've created one for you here. Thought it might be helpful to see how it looks:
https://haystack-home-s4u5h2xbs-deepset-overnice.vercel.app/tutorials/19_basic_multimodal_search_pipeline

Copy link
Member

@TuanaCelik TuanaCelik left a comment

Choose a reason for hiding this comment

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

Is there any reason why the tutorial itself is not a .ipynb ? By default we (for now) allow people to open the tutorial in colab. So if this is what you also want people to be able to do the tutorial itself should be a .ipynb file and then the generate_markdown.py script will generate the displayed markdown for you anyway.
You can see the contributing process here: https://github.com/deepset-ai/haystack-tutorials/blob/main/Contributing.md

@mayankjobanputra
Copy link
Contributor Author

mayankjobanputra commented Oct 27, 2022

Is there any reason why the tutorial itself is not a .ipynb ? By default we (for now) allow people to open the tutorial in colab. So if this is what you also want people to be able to do the tutorial itself should be a .ipynb file and then the generate_markdown.py script will generate the displayed markdown for you anyway.

@TuanaCelik It is .ipynb at least in the PR when I see it under Files changed. Maybe I am missing it at some place?

@TuanaCelik
Copy link
Member

Is there any reason why the tutorial itself is not a .ipynb ? By default we (for now) allow people to open the tutorial in colab. So if this is what you also want people to be able to do the tutorial itself should be a .ipynb file and then the generate_markdown.py script will generate the displayed markdown for you anyway.

It is .ipynb at least in the PR when I see it under Files changed. Maybe I am missing it at some place?

ah my mistake, then there's something else weird here. The generated code snippets in the preview link look odd, there's no highlighting. Maybe there's something else that's going on here. Did you generate the .md file with the script or manually?

@mayankjobanputra
Copy link
Contributor Author

Is there any reason why the tutorial itself is not a .ipynb ? By default we (for now) allow people to open the tutorial in colab. So if this is what you also want people to be able to do the tutorial itself should be a .ipynb file and then the generate_markdown.py script will generate the displayed markdown for you anyway.

It is .ipynb at least in the PR when I see it under Files changed. Maybe I am missing it at some place?

ah my mistake, then there's something else weird here. The generated code snippets in the preview link look odd, there's no highlighting. Maybe there's something else that's going on here. Did you generate the .md file with the script or manually?

I generated it using the script.

Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

Hey @mayankjobanputra, great work! Concise and clear. I especially like the additional queries 😄

I've spotted a couple of typos and phrasing, extended some variable names (i love long variable names 😄). I had to comment on the .md because the .ipynb doesn't have its cells cleared, so it's huge and hard to parse by a human. Not a big deal though, just move my suggestions there. I think you should clear the cells before merging this one to main.

markdowns/19_Basic_multimodal_search_pipeline.md Outdated Show resolved Hide resolved
markdowns/19_Basic_multimodal_search_pipeline.md Outdated Show resolved Hide resolved
markdowns/19_Basic_multimodal_search_pipeline.md Outdated Show resolved Hide resolved
markdowns/19_Basic_multimodal_search_pipeline.md Outdated Show resolved Hide resolved
markdowns/19_Basic_multimodal_search_pipeline.md Outdated Show resolved Hide resolved
markdowns/19_Basic_multimodal_search_pipeline.md Outdated Show resolved Hide resolved
markdowns/19_Basic_multimodal_search_pipeline.md Outdated Show resolved Hide resolved
markdowns/19_Basic_multimodal_search_pipeline.md Outdated Show resolved Hide resolved
markdowns/19_Basic_multimodal_search_pipeline.md Outdated Show resolved Hide resolved
markdowns/19_Basic_multimodal_search_pipeline.md Outdated Show resolved Hide resolved
@ZanSara ZanSara added the new tutorial request for new tutorial label Oct 28, 2022
index.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

Can you also add an entry in the README.md file?

@brandenchan brandenchan self-requested a review November 3, 2022 15:50
Copy link
Contributor

@brandenchan brandenchan left a comment

Choose a reason for hiding this comment

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

Added a few smaller comments, but otherwise I give this the thumbs up

Copy link
Contributor

@agnieszka-m agnieszka-m left a comment

Choose a reason for hiding this comment

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

Just a minor typo and it's good to go as far as I'm concerned :)

@mayankjobanputra mayankjobanputra merged commit 3a1ad22 into main Nov 7, 2022
@mayankjobanputra mayankjobanputra deleted the tut19_multimodal branch November 7, 2022 09:33
@mayankjobanputra mayankjobanputra self-assigned this Nov 7, 2022
@ZanSara ZanSara removed their request for review July 5, 2023 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new tutorial request for new tutorial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants