Skip to content

Structured Outputs with Instructor #247

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

Merged
merged 5 commits into from
Jun 20, 2024
Merged

Structured Outputs with Instructor #247

merged 5 commits into from
Jun 20, 2024

Conversation

HamzaFarhan
Copy link
Contributor

Using Gemini Flash with Instructor for structured outputs as assigned to me in #240.

Changes

How I tested this

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@elijahbenizzy elijahbenizzy self-requested a review June 20, 2024 18:07
Copy link
Contributor

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

Looks good! A few minor changes then we can merge!

],
writes=["generated_topic", "chat_history"],
)
def creator(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add docstring comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding just short docstring comments? But to each one? E.G. the first sentence of this, but for each action. Makes it easier to read/undersatnd the example :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. My bad. Added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, specifically I meant docstrings to the functions decorated by @action -- These are the important ones that tell the story. Think you have enough comments in this function!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 📝

@elijahbenizzy
Copy link
Contributor

Also you'll want to run pre-commit:

pre-commit run --all

@HamzaFarhan
Copy link
Contributor Author

Done @elijahbenizzy

@elijahbenizzy
Copy link
Contributor

Done @elijahbenizzy

Great, thanks, one more comment about docs then I can take over and get the CI to pass (external users aren't allowed to run it now so I can just do this quickly). Then when we merge we can merge this! #248

@elijahbenizzy
Copy link
Contributor

This looks great! Thank you so much. I'm going to:

  1. Merge to main
  2. Fix some of the CI
  3. Merge the contributor PR!

Really appreciate your contribution 🙏

Copy link
Contributor

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

Thank you! As discussed, merging to main then I'll fix real quick.

@elijahbenizzy elijahbenizzy merged commit 66cc1d5 into apache:main Jun 20, 2024
6 of 11 checks passed
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.

2 participants