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.

Sign up for GitHub

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

Jump to bottom

feat: ImageToText (caption generator) #3859

Merged
merged 15 commits into from
Jan 23, 2023
Merged

feat: ImageToText (caption generator) #3859

merged 15 commits into from
Jan 23, 2023

Conversation

anakin87
Copy link
Member

@anakin87 anakin87 commented Jan 14, 2023 •
edited
Loading

Related Issues

Proposed Changes:

ImageToText would be a node that takes a list of paths to images and captions them.

Only a first draft to run the CI...

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added tests that demonstrate the correct behavior of the change
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

@anakin87
Copy link
Member Author

Hey @ZanSara, here's a first draft for this node!

@anakin87 anakin87 marked this pull request as ready for review January 16, 2023 15:00
@anakin87 anakin87 requested a review from a team as a code owner January 16, 2023 15:00
@anakin87 anakin87 requested review from ZanSara and removed request for a team January 16, 2023 15:00
@anakin87 anakin87 marked this pull request as draft January 16, 2023 16:10
@anakin87 anakin87 marked this pull request as ready for review January 16, 2023 16:10
ZanSara
ZanSara reviewed Jan 17, 2023
View reviewed changes
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.

Very nice! No serious remarks on the arch, just a couple of nitpicks.

By the way: right now we can go ahead with this node, as we discussed its details way before the proposals process was in place. For the next one, let's follow it! New nodes are the best usecases for them. I'll do my best to fast-track your proposals, so we can move from proposal to implementation in matter of a few days and give everyone the chance to add their thoughts on it 🙂

haystack/nodes/image_to_text/base.py Outdated Show resolved Hide resolved
haystack/nodes/image_to_text/transformers.py Outdated Show resolved Hide resolved
haystack/nodes/image_to_text/base.py Outdated Show resolved Hide resolved
@anakin87
Copy link
Member Author

I added two tests.
Should we test something else?

Other aspects to improve?

@anakin87 anakin87 requested a review from ZanSara January 17, 2023 22:23
@anakin87 anakin87 marked this pull request as draft January 17, 2023 23:10
@anakin87 anakin87 marked this pull request as ready for review January 17, 2023 23:10
ZanSara
ZanSara reviewed Jan 18, 2023
View reviewed changes
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.

Really good! We can expand the test suite a bit and then it's ready to go 😊

test/nodes/test_image_to_text.py
"a red apple is sitting on a pile of hay",
"a white car parked in a parking lot",
"a cat laying in the grass",
"a blurry photo of a blurry shot of a black object",
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this model has no concept of galaxies 😆

test/nodes/test_image_to_text.py Outdated Show resolved Hide resolved
test/nodes/test_image_to_text.py Outdated Show resolved Hide resolved
haystack/nodes/image_to_text/transformers.py Outdated Show resolved Hide resolved
@anakin87 anakin87 marked this pull request as draft January 19, 2023 08:10
@anakin87 anakin87 marked this pull request as ready for review January 19, 2023 08:10
@anakin87
Copy link
Member Author

@ZanSara mypy failure seems not related to this PR.
How to solve this issue?

@ZanSara
Copy link
Contributor

ZanSara commented Jan 19, 2023

Yes we're having the same issue in a ton of places 🙈 I'm gonna take care of it and merge as soon as I sort it out. Sorry for the mess!

@anakin87 anakin87 requested a review from ZanSara January 19, 2023 19:07
ZanSara
ZanSara approved these changes Jan 23, 2023
View reviewed changes
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.

Great, thank you! 😊

@ZanSara ZanSara merged commit b910df7 into deepset-ai:main Jan 23, 2023
@anakin87 anakin87 deleted the image_to_text branch January 23, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZanSara ZanSara ZanSara approved these changes

Assignees
No one assigned
Labels
topic:images topic:indexing type:feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anakin87 @ZanSara