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

[DRAFT] FEAT New HTTP Target #392

Closed
wants to merge 44 commits into from
Closed

[DRAFT] FEAT New HTTP Target #392

wants to merge 44 commits into from

Conversation

jbolor21
Copy link
Contributor

@jbolor21 jbolor21 commented Sep 24, 2024

Description

Closing this one due to merge conflict messups :)

Adding a new target for endpoints without APIs (like Bing Image Creator). This is updated from two closed PR: #350
#351

Also to make this work the SearchReplaceConverter was altered to use regex rather than .replace(). This allows for more flexibility in replacing characters/strings.

Tests and Documentation

Added notebook to test AOAI and BIC endpoints using HTTP Target and added unit tests

@jbolor21 jbolor21 marked this pull request as draft September 24, 2024 14:22
@jbolor21 jbolor21 marked this pull request as ready for review October 9, 2024 01:01
Bolor-Erdene Jagdagdorj added 2 commits October 8, 2024 18:03
follow_redirects=True, # This is defaulted to true but using requests over httpx for this reason
)

if self.callback_function:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good!

@nina-msft nina-msft changed the title [DRAFT] FEAT New HTTP Target FEAT New HTTP Target Oct 9, 2024

{{
"messages": [
{{"role": "user", "content": "{{PROMPT}}"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can you use {PLACEHOLDER_PROMPT} in the AOAI example since it's the default for prompt_regex_string? Then you don't need to provide the prompt_regex_string but can just add a comment saying this is the default.

Feel free to keep {PROMPT} for the BIC example.

Copy link
Contributor

Choose a reason for hiding this comment

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

...can disregard this if you choose to make the default {PROMPT}. I think the original reason why I made this suggestion was because prompt and PROMPT but I think it may be clear enough that the placeholder is not the same as the variable. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol is there a strong preference just addressed @rlundeen2 comment saying to use "{PROMPT}" as the placeholder - either way is fine just two different notes ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but agree yeah I can remove the prompt_regex_string if I use the default!

import re


logger = logging.getLogger(__name__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Curiosity, why did you choose to make a separate folder for this target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rich's suggestion! But this way we can put all the parsing functions in a subdirectory!

@jbolor21 jbolor21 marked this pull request as draft October 9, 2024 22:19
@jbolor21 jbolor21 changed the title FEAT New HTTP Target [DRAFT] FEAT New HTTP Target Oct 9, 2024
@jbolor21 jbolor21 closed this Oct 9, 2024
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.

3 participants