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

Mask API key for Anyscale chat model #12419

Conversation

aidoskanapyanov
Copy link
Contributor

Description: Add masking of API Key for Anyscale chat model when printed.
Issue: #12165
Dependencies: None
Tag maintainer: @eyurtsev

@vercel
Copy link

vercel bot commented Oct 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Oct 30, 2023 3:22am

@dosubot dosubot bot added Ɑ: models Related to LLMs or chat model modules 🤖:improvement Medium size change to existing code to handle new use-cases labels Oct 27, 2023
@aidoskanapyanov aidoskanapyanov force-pushed the i12165-mask-anyscale-chat-model-api-key branch from 8089afb to 5f63da9 Compare October 27, 2023 11:16
@eyurtsev eyurtsev self-requested a review October 29, 2023 02:39
@@ -67,12 +74,13 @@ def lc_secrets(self) -> Dict[str, str]:

@staticmethod
def get_available_models(
anyscale_api_key: Optional[str] = None,
anyscale_api_key: Optional[SecretStr] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The static method is public, let's accept Optional[Union[str, SecretStr]] and leverage _to_secret once again to avoid breaking API for any existing users that rely on the staticmethod directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed this comment this new commit dcbb17f

@eyurtsev
Copy link
Collaborator

Looks good we let's update the static method to also handle str as an input and should be able to merge then

)


@pytest.mark.requires("openai")
Copy link
Collaborator

Choose a reason for hiding this comment

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

one more thing that i noticed -- anyscale doesn't use openai under the hood, so we don't need to use requirs("openai")

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh sorry totally wrong -- yes it does use openai, so this is fine

@aidoskanapyanov
Copy link
Contributor Author

Looks good we let's update the static method to also handle str as an input and should be able to merge then

@eyurtsev I've addressed this comment this new commit dcbb17f

@eyurtsev eyurtsev self-requested a review November 1, 2023 13:37
@eyurtsev
Copy link
Collaborator

eyurtsev commented Nov 1, 2023

@aidoskanapyanov i think we can close this one since you've already taken care of the code in: #12406 ?

@eyurtsev eyurtsev closed this Nov 1, 2023
@aidoskanapyanov
Copy link
Contributor Author

@eyurtsev ok 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:improvement Medium size change to existing code to handle new use-cases Ɑ: models Related to LLMs or chat model modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants