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

chore: Include types to instructor.distil and tests #396

Merged
merged 7 commits into from
Feb 8, 2024

Conversation

savarin
Copy link
Contributor

@savarin savarin commented Feb 3, 2024

This PR adds types to the following files, as per #390.

instructor/distil.py
tests/test_distil.py
Ellipsis 🚀 This PR description was created by Ellipsis for commit 0050747.

Summary:

This PR enhances type safety by adding type hints to the instructor.distil module and its corresponding tests, and updates the mypy.yml workflow to include these files.

Key points:

  • Added type hints to the instructor.distil module and its corresponding tests.
  • Updated the mypy.yml workflow file to include type checking for instructor/distil.py and tests/test_distil.py.

Generated with ❤️ by ellipsis.dev

@savarin savarin changed the title Include types 4 [WIP] Include types 4 Feb 3, 2024
@savarin savarin changed the title [WIP] Include types 4 Include types to instructor.distil and tests Feb 5, 2024
msg = f"Return type hint for {fn} must subclass `pydantic.BaseModel'"
assert is_return_type_base_model_or_instance(fn), msg
return_base_model = inspect.signature(fn).return_annotation

@functools.wraps(fn)
def _dispatch(*args, **kwargs):
def _dispatch(*args: Any, **kwargs: Any) -> Callable[..., T_Retval]:
name = name if name else fn.__name__
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note change here to include name when null, since openai_kwargs requires non-null name. Alternative would be to set assert name is not None.

@savarin savarin marked this pull request as ready for review February 5, 2024 07:51
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me!


Something look wrong? You can customize Ellipsis by editing the ellipsis.yaml for this repository.

Generated with ❤️ by ellipsis.dev

@savarin savarin force-pushed the include-types-4 branch 4 times, most recently from e154310 to 0328afd Compare February 6, 2024 00:01
@jxnl jxnl changed the title Include types to instructor.distil and tests chore: Include types to instructor.distil and tests Feb 6, 2024
@savarin savarin force-pushed the include-types-4 branch 3 times, most recently from c56d1af to 40078f6 Compare February 6, 2024 21:06
@savarin savarin force-pushed the include-types-4 branch 2 times, most recently from 5ba62e9 to 47c9fd7 Compare February 7, 2024 05:03
@jxnl
Copy link
Collaborator

jxnl commented Feb 8, 2024

Don't worry about the Docs one. Is this good to merge?

@savarin
Copy link
Contributor Author

savarin commented Feb 8, 2024

Don't worry about the Docs one. Is this good to merge?

@jxnl OK will do, just puzzled why main passes but my branch doesn't.

It's good to merge.

@jxnl jxnl merged commit d632682 into instructor-ai:main Feb 8, 2024
7 of 8 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