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

feat(bing): add image recognition functionality (#452) #11

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

danny-avila
Copy link
Owner

@Richard-Weiss Please review this PR as I've forked node-gpt-api to keep this library better maintained

Add groundwork for implementing the image recognition feature.
The only missing component is setting the parameters in the message dynamically.
I would appreciate any assistance.
--Edit--
I've added receiving the necessary values from the body of the /conversation request.
The imageURL or imageBase64 are added dynamically by for example PandoraAI.
Tested with image URL and direct base64 string.

@Richard-Weiss
Copy link
Contributor

Richard-Weiss commented Oct 19, 2023

@danny-avila
Why do you need a review to merge a PR in your own fork into the main branch of your own fork?
Or did I get something wrong?
This seems a bit odd.
I can approve it but next time you can just create a new local branch from my pr and merge it locally into your main branch.
No need for any approval as it isn't being merged into the upstream repo.
Also I see that you got the wrong PR I think.
You can google how to fetch a pr and merge it locally or I can show it to you tomorrow.

@danny-avila
Copy link
Owner Author

danny-avila commented Oct 19, 2023

Why do you need a review to merge a PR in your own fork into the main branch of your own fork?
Or did I get something wrong?
This seems a bit odd.

I'm looking to diverge from upstream since it seems the maintainer has no interest in maintaining it. The goal here is not simply to have my own fork but to ensure the community can keep maintaining the original project

I can approve it but next time you can just create a new local branch from my pr and merge it locally into your main branch.
No need for any approval as it isn't being merged into the upstream repo.

There is "no need", but as this repo is publishing to npm and already at 600 weekly downloads for this week, I would appreciate a second look/test.

Also I see that you are missing the actual important commit.
You can google how to fetch a pr and merge it locally or I can show it to you tomorrow.

Does your PR on the upstream not have waylaidwanderer/node-chatgpt-api@187085f as the latest commit? it's here

@Richard-Weiss
Copy link
Contributor

Richard-Weiss commented Oct 19, 2023

Yeah, but I thought you just wanted to merge the two PRs into this fork, or what exactly do you want to do?
It's a bit late for me so I'll write tomorrow.
But in general I would suggest that you create two branches for 451 and 452 and two PRs.
It seems like this name is for 452 but the actual commits are from 451.
I don't really know what me reviewing it would accomplish, as it's a bit silly to review your own code, but I can check how this Repo would work with the PR if that's what you want.

@Richard-Weiss
Copy link
Contributor

Richard-Weiss commented Oct 26, 2023

@danny-avila I'll create a new PR only containing the relevant commits for the image recognition.
I'll tag you once it's done.
--Edit--
I can't really figure out how to create a new PR in this repo from my branch in my repo.
However I think you should be able to pull the branch "bing-image-recognition" from my fork and create a new PR with it.

I also have a feature in my fork for using Local LLMs, is this something you or other users might be interested in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants