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.

Sign up for GitHub

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

Jump to bottom

SecretStr for fireworks api #12475

Merged
merged 12 commits into from
Oct 31, 2023
Merged

SecretStr for fireworks api #12475

merged 12 commits into from
Oct 31, 2023

Conversation

nepalprabin
Copy link
Contributor

@nepalprabin nepalprabin commented Oct 28, 2023 •
edited
Loading

  • Description: This pull request removes secrets present in raw format,
  • Issue: Fireworks api key was exposed when printing out the langchain object #12165
  • Maintainer: @eyurtsev

@vercel
Copy link

vercel bot commented Oct 28, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
langchain ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 31, 2023 1:16am

@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 28, 2023
@nepalprabin nepalprabin mentioned this pull request Oct 28, 2023
Closed
40 tasks
eyurtsev
eyurtsev reviewed Oct 29, 2023
View reviewed changes
Copy link
Collaborator

@eyurtsev eyurtsev left a comment

Choose a reason for hiding this comment

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

Looks great! Let's change the requires to use fireworks rather than openai and should be ready to merge!

libs/langchain/tests/unit_tests/chat_models/test_fireworks.py Outdated
assert isinstance(llm.fireworks_api_key, SecretStr)


@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.

We should require fireworks rather than openai

Copy link
Contributor Author

@nepalprabin nepalprabin Oct 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When using

@pytest.mark.requires("fireworks")
def test_api_key_masked_when_passed_from_env(
    monkeypatch: MonkeyPatch, capsys: CaptureFixture
) -> None:
    monkeypatch.setenv("FIREWORKS_API_KEY", "secret-api-key")
    llm = ChatFireworks()
    print(llm.fireworks_api_key, end="")
    captured = capsys.readouterr()

    assert captured.out == "**********"

my test fails, but it gets passed when using openai. But all other testcase gets passed. Am I missing something here?
@eyurtsev

@eyurtsev eyurtsev self-requested a review October 29, 2023 02:29
@baskaryan baskaryan added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Oct 30, 2023
@baskaryan baskaryan merged commit b109cb0 into langchain-ai:master Oct 31, 2023
20 checks passed
xieqihui pushed a commit to xieqihui/langchain that referenced this pull request Nov 21, 2023
@nepalprabin @baskaryan @xieqihui
SecretStr for fireworks api (langchain-ai#12475)
4999d1e 
- **Description:** This pull request removes secrets present in raw
format,
- **Issue:** Fireworks api key was exposed when printing out the
langchain object
[langchain-ai#12165](langchain-ai#12165)
 - **Maintainer:** @eyurtsev

---------

Co-authored-by: Bagatur <[email protected]>
hoanq1811 pushed a commit to hoanq1811/langchain that referenced this pull request Feb 2, 2024
@nepalprabin @baskaryan
SecretStr for fireworks api (langchain-ai#12475)
a80b200 
- **Description:** This pull request removes secrets present in raw
format,
- **Issue:** Fireworks api key was exposed when printing out the
langchain object
[langchain-ai#12165](langchain-ai#12165)
 - **Maintainer:** @eyurtsev

---------

Co-authored-by: Bagatur <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baskaryan baskaryan baskaryan approved these changes

@eyurtsev eyurtsev Awaiting requested review from eyurtsev

Assignees
No one assigned
Labels
🤖:improvement Medium size change to existing code to handle new use-cases lgtm PR looks good. Use to confirm that a PR is ready for merging. Ɑ: models Related to LLMs or chat model modules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nepalprabin @eyurtsev @baskaryan