Skip to content

Add/vllm image #9

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

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

Add/vllm image #9

wants to merge 9 commits into from

Conversation

mina-parham
Copy link
Contributor

No description provided.

@@ -40,6 +45,7 @@ def __init__(
limit_worker_concurrency: int,
no_register: bool,
conv_template: str,
temp_img_dir: str,
Copy link
Member

Choose a reason for hiding this comment

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

Why do you pass the temp directory in? I notice down further you delete it which seems pretty drastic...so maybe this should be something owned by this library? Or maybe I misunderstood something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that we need a directory to save images, and we also need to give vllm access to that directory. so I wanted it to be defined per plugin instead of using a fixed directory. That’s why I’ll pass an image_dir to the OpenAI proxy from each plugin that calls it, so it can handle and save images within that plugin.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm...this feels unusual, and I'm scared of deleting a directory on my hard drive accidentally. 😅

What if OpenAIProxyWorker made the directory and then there's a function to get the temp directory that plugins can call and pass to vllm/whatever? Would that work for the order of operations?

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