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

Added discussion to tutorial showing a dry setup for anthropic #901

Merged
merged 4 commits into from
Mar 3, 2025

Conversation

maykcaldas
Copy link
Collaborator

No description provided.

@maykcaldas maykcaldas self-assigned this Mar 3, 2025
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. documentation Improvements or additions to documentation labels Mar 3, 2025
Copy link
Collaborator

@jamesbraza jamesbraza left a comment

Choose a reason for hiding this comment

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

Nice

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 3, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This pull request updates the tutorial to include configuration for Anthropic’s API keys and adjusts the printed output to clearly indicate whether environment variables are set.

  • Updated .env file instructions and print statements to support Anthropic API keys
  • Revised embedding model configuration and added an extended example for clarity
  • Updated pre-commit hooks to reorganize the jupytext configuration

Reviewed Changes

File Description
docs/tutorials/settings_tutorial.md Updated .env instructions, print statements, and embedding configuration
.pre-commit-config.yaml Reorganized the jupytext hook configuration
docs/tutorials/settings_tutorial.ipynb Reflected markdown changes in the notebook format

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

docs/tutorials/settings_tutorial.ipynb:58

  • The f-string in the notebook cell contains nested double quotes that will trigger a syntax error. Replace the inner double quotes with single quotes (e.g., f"OPENAI_API_KEY: {'is set' if os.environ['OPENAI_API_KEY'] else 'is not set'}") to fix the issue.
    f"OPENAI_API_KEY:    {"is set" if os.environ['OPENAI_API_KEY'] else "is not set"}"

@maykcaldas maykcaldas requested a review from Copilot March 3, 2025 18:43
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR updates the tutorial to demonstrate a dry setup for Anthropic models in PaperQA by revising the configuration examples and environment variable output messaging.

  • Updated tutorial text and code examples to clarify the setup with OpenAI and Anthropic models.
  • Adjusted environment variable print statements to conditionally indicate whether keys are set.
  • Synchronized changes across the Markdown tutorial, its corresponding Jupyter Notebook, and updated the pre-commit configuration.

Reviewed Changes

File Description
docs/tutorials/settings_tutorial.md Updated tutorial instructions and code examples for model setup clarity.
.pre-commit-config.yaml Revised jupytext configuration for syncing notebooks to Markdown.
docs/tutorials/settings_tutorial.ipynb Mirrored code example changes from the Markdown file into the notebook.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

docs/tutorials/settings_tutorial.ipynb:58

  • This line suffers from the same nested double quotes issue within the f-string. Replace inner double quotes with single quotes to ensure correct syntax.
    f"OPENAI_API_KEY:    {"is set" if os.environ['OPENAI_API_KEY'] else "is not set"}"

docs/tutorials/settings_tutorial.ipynb:61

  • The f-string here contains nested double quotes that will lead to a syntax error. Use single quotes for the inner string values to resolve the issue.
    f"ANTHROPIC_API_KEY: {"is set" if os.environ['ANTHROPIC_API_KEY'] else "is not set"}"

@maykcaldas maykcaldas requested a review from Copilot March 3, 2025 21:10

Choose a reason for hiding this comment

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

PR Overview

This PR enhances the tutorial by updating the setup instructions to support both OpenAI and Anthropic API keys via environmental configuration and by providing a more verbose example for clarity. Key changes include:

  • Updating the instructions for creating the .env file to include Anthropic API keys.
  • Revising the code snippets to print more descriptive messages about whether API keys are set.
  • Adjusting the pre-commit configuration to change how notebooks are converted with jupytext.

Reviewed Changes

File Description
docs/tutorials/settings_tutorial.md Updated comments, environment variable handling, and embedding configuration example.
.pre-commit-config.yaml Revised jupytext hook arguments to modify notebook conversion behavior.
docs/tutorials/settings_tutorial.ipynb Synchronized changes from the Markdown tutorial in the notebook version.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

.pre-commit-config.yaml:85

  • [nitpick] The updated jupytext hook now uses '--to, md' instead of '--sync'. Please verify that this change correctly achieves the intended notebook conversion workflow.
args: [--to, md, --pipe, black]

docs/tutorials/settings_tutorial.md:276

  • [nitpick] The embedding setting was reverted from 'hybrid-st-multi-qa-MiniLM-L6-cos-v1' to 'st-multi-qa-MiniLM-L6-cos-v1' without additional context. Consider adding a clarifying comment to explain this change for future maintainers.
settings.embedding = "st-multi-qa-MiniLM-L6-cos-v1"
@maykcaldas maykcaldas enabled auto-merge (squash) March 3, 2025 21:12
@maykcaldas maykcaldas disabled auto-merge March 3, 2025 21:22
@maykcaldas maykcaldas merged commit 27ef4e8 into main Mar 3, 2025
5 of 7 checks passed
@maykcaldas maykcaldas deleted the update-setting-tutorial branch March 3, 2025 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants