-
Notifications
You must be signed in to change notification settings - Fork 17
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
Feature/azure openai #42
base: main
Are you sure you want to change the base?
Conversation
|
||
export OPENAI_URL="${endpoint}/openai/deployments/${deployment-id}" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand - do you want to set endpoint and deployment ID every time you call please - or at least with every new terminal? Wouldn't that be a lot of work? We could also store these values somewhere on the disk, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saving to disk sounds like the right thing to do.
But as the script does not yet do so I didn't make assumptions on how to implement this, but rather start this conversation.
- how should we save to disk?
~/.pleaserc
~/.please/config
- other?
- when implementing a config/storage on disk... is there anything of the current state to be migrated?
- eg:
debug
ormodel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~/.pleaserc
seems like a good location.
I would not sabe model
or debug
since they are rather transient attributes you might want to change every invocation.
please.sh
Outdated
@@ -23,6 +23,7 @@ questionMark="\x1B[31m?\x1B[0m" | |||
checkMark="\x1B[31m\xE2\x9C\x93\x1B[0m" | |||
|
|||
openai_invocation_url=${OPENAI_URL:-"https://api.openai.com/v1"} | |||
openai_use_azure_endpoint=$(echo "$OPENAI_URL" | grep azure.com >/dev/null && echo 1 || echo 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a bit brittle - should we rather set it wenn setting the OPENAI_URL as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather infer the information from the actually used endpoint url rather than keep the 2 settings in sync.
I can change the code to be more robust in a proper if
block.
But if you prefer a setting on it's own (saved to disk), I can do that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, if we make it a bit more robust.
I just reviewed the changes. I think the script could be a bit easier and it could also store the necessary attributes so you don't have to enter them so often. |
Did you also sign your commits? |
not yet. |
No description provided.