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

Update mode.py to warn in Mode.FUNCTIONS access vs. in __new__ #678

Merged
merged 1 commit into from
May 19, 2024

Conversation

boydgreenfield
Copy link
Contributor

@boydgreenfield boydgreenfield commented May 17, 2024

Currently, any import of instructor raises a DeprecationWarning. This is noisy and also makes it impossible to have test suites raise an error on DeprecationWarning. Example code:

import warnings
warnings.simplefilter("default")

import instructor  # raises `DeprecationWarning: FUNCTIONS is deprecated and will be removed in future versionss enum_member = enum_class._new_member_(enum_class, *args)`

🚀 This description was created by Ellipsis for commit 90edc70

Summary:

This PR moves the deprecation warning for FUNCTIONS in Mode class to only trigger when accessed, reducing import-time noise.

Key points:

  • Moved DeprecationWarning for FUNCTIONS from __new__ to __getattribute__ in a Mode metaclass.
  • Reduces warning noise at import time.
  • Allows better handling of DeprecationWarning in test suites.

Generated with ❤️ by ellipsis.dev


🚀 This description was created by Ellipsis for commit 9ef6f1a

Summary:

This PR moves the deprecation warning for FUNCTIONS in the Mode class to only trigger when accessed, reducing import-time noise.

Key points:

  • Moved DeprecationWarning for FUNCTIONS from __new__ to __getattribute__ in Mode class using _WarnOnFunctionsAccessEnumMeta metaclass.
  • Reduces warning noise at import time.
  • Allows better handling of DeprecationWarning in test suites.

Generated with ❤️ by ellipsis.dev

Copy link

sweep-ai bot commented May 17, 2024

Sweep: PR Review

instructor/mode.py

The __new__ method was removed and replaced with a __getattr__ method to issue a deprecation warning for the FUNCTIONS attribute upon access rather than instantiation.

No issues found with the reviewed changes


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.

❌ Changes requested. Reviewed everything up to 90edc70 in 2 minutes and 8 seconds

More details
  • Looked at 33 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_1n2HvCQH5zZpIZX4


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

instructor/mode.py Outdated Show resolved Hide resolved
@boydgreenfield boydgreenfield marked this pull request as draft May 17, 2024 20:21
Copy link
Contributor

ellipsis-dev bot commented May 17, 2024

This is a cross repository pull request, but Ellipsis isn't installed in boydgreenfield/instructor. In order to have Ellipsis address comments in this PR, you'll need to install Ellipsis in that repository.

@boydgreenfield boydgreenfield force-pushed the patch-1 branch 2 times, most recently from 6cf9f85 to b8168e5 Compare May 17, 2024 20:44
Currently, any import of `instructor` raises a `DeprecationWarning`. This is noisy and also makes it impossible to have test suites raise an error on `DeprecationWarning`. Example code:

```python3
import warnings
warnings.simplefilter("default")

import instructor  # raises `DeprecationWarning: FUNCTIONS is deprecated and will be removed in future versions enum_member = enum_class._new_member_(enum_class, *args)`
```
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! Reviewed everything up to 9ef6f1a in 52 seconds

More details
  • Looked at 43 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. instructor/mode.py:6
  • Draft comment:
    The implementation of the deprecation warning in __getattribute__ is correct and should effectively warn users when FUNCTIONS is accessed. However, ensure that all other enum members are accessible without any unintended side effects or errors by thoroughly testing access to all other members of the Mode enum.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The PR moves the deprecation warning for the FUNCTIONS enum member from the __new__ method to the __getattribute__ method of a custom metaclass _WarnOnFunctionsAccessEnumMeta. This change is intended to reduce the noise of deprecation warnings at import time, only triggering them when the FUNCTIONS member is actually accessed. This is a good approach as it allows for more controlled handling of deprecation warnings, especially in test environments where warnings might be treated as errors or need special handling.

Workflow ID: wflow_KxL4gpsmY7MmwEvo


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@jxnl jxnl merged commit 00db862 into jxnl:main May 19, 2024
2 of 12 checks passed
@boydgreenfield boydgreenfield deleted the patch-1 branch May 20, 2024 13:54
ssonal added a commit to ssonal/instructor that referenced this pull request May 22, 2024
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.

None yet

2 participants