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

🚀 [Feature] Add deprecated Decorator #3161

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

yhna940
Copy link
Contributor

@yhna940 yhna940 commented Oct 12, 2024

What does this PR do?

Summary

This PR introduces a deprecated decorator, designed to mark functions as deprecated with detailed warnings, following the approach in torch/onnx/_deprecation.py. The decorator provides a standard way to issue deprecation warnings and update docstrings, informing users of the version when a function was deprecated, its removal target, and recommended alternative actions.

Changes

  • New deprecated decorator: Adds a decorator that marks functions as deprecated, integrates a warning when the function is called, and updates docstrings accordingly.
  • Integration with FindTiedParametersResult: Applied the deprecated decorator to the values method in the FindTiedParametersResult class, signaling to users that this method will be removed in Accelerate v1.3.0 and suggesting alternative approaches.

Example Usage

After this PR, the following usage:

from accelerate.utils.modeling import FindTiedParametersResult
results = FindTiedParametersResult([[1, 2, 3]])
results.values()

will produce output similar to:

<stdin>:1: FutureWarning: 'accelerate.utils.modeling.values' is deprecated in version 1.0.0rc0 and will be removed in 1.3.0. Please use another method instead.
[2, 3]

Testing

Included a test for deprecated to validate:

  • Warning output when a deprecated function is called.
  • Docstring formatting to ensure it includes the deprecation note with the appropriate information.

@BenjaminBossan
Copy link
Member

Maybe not relevant, but Python added its own warnings.deprecated decorator in Python 3.13. Using the same name could be confusing. The Python decorator apparently also allows type checkers to flag the usage, which would be nice to have. Of course, we can't use it directly since we want to support older Python versions.

@yhna940
Copy link
Contributor Author

yhna940 commented Oct 16, 2024

Maybe not relevant, but Python added its own warnings.deprecated decorator in Python 3.13. Using the same name could be confusing. The Python decorator apparently also allows type checkers to flag the usage, which would be nice to have. Of course, we can't use it directly since we want to support older Python versions.

Hello @BenjaminBossan, thank you for the insightful review! I wasn’t aware of the warnings.deprecated decorator introduced in Python 3.13, and I appreciate you pointing it out. As I am currently working with a lower Python version (likely 3.8, according to the Dev Container), I implemented a custom decorator to ensure compatibility.

That said, the name overlap could indeed be confusing. Would using an alternative name for the decorator help clarify things? I’d be glad to make adjustments if needed!

@BenjaminBossan
Copy link
Member

As to the Python version, 3.8 is going to be end of life by the end of the month, so the containers will be updated soon. Still, it will be quite some time before 3.13 will be the lowest supported Python version, so we can't assume that the decorator exists. More importantly, I wonder if we can "borrow" some of the implementation details of warnings.deprecated, as I'm sure the Python devs put a lot of thought into it. Regarding the naming, I'll let Zach decide on that.

Copy link
Collaborator

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Thanks, I think what I'd like to see done here is we keep the same namings, and if we are on the upper-bound python version that has it (3.13), we use that one instead. How does that sound? Otherwise this looks good to me

(Sorry it took a bit to get to this PR!)

@yhna940
Copy link
Contributor Author

yhna940 commented Oct 31, 2024

Thank you for your feedback, I agree with your suggestions regarding naming and compatibility! Using Python’s warnings.deprecated decorator in 3.13 and above, while defaulting to our custom decorator in lower versions, sounds like a balanced approach. This way, we maintain compatibility across versions while taking advantage of the built-in decorator when possible.

I’ve resolved the conflicts in the code for now, and I’ll be sure to apply any further adjustments based on additional feedback. Thanks again!

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I only have a nit left, no blocker.

Using Python’s warnings.deprecated decorator in 3.13 and above, while defaulting to our custom decorator in lower versions

I don't think we really need to switch over come Python 3.13, since the Python decorator works quite differently from this one.

removed_in (`str`):
The version when the function will be removed.
instructions (`str`):
The action users should take.
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit: I'd find it more intuitive if the instructions was renamed instruction and if the "Please " was not added in front, as I would assume as a caller that I need to pass a full sentence here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a7a43aa and 2cb4648 Thank you :)

tests/test_utils.py Show resolved Hide resolved
@matthewdouglas
Copy link
Member

Hi folks,

Could we consider using the implementation from typing_extensions>=4.5 instead? It could maybe be wrapped still to provide help with formatting for UX/docs, but that way we get improved IDE support and alignment with PEP 702.

@BenjaminBossan
Copy link
Member

@matthewdouglas AFAICT, that implementation is a backport of warnings.deprecated that we discussed above. I agree that typing support would be nice to have, but feature-wise, the two are not on par, so not sure if we can simply replace it.

@yhna940
Copy link
Contributor Author

yhna940 commented Nov 22, 2024

I hope you're all doing well! I wanted to follow up on this PR to check if there are any remaining concerns or additional feedback that I should address to move it forward. Please let me know if there's anything else I can do to improve this PR :) Thank you for your time and guidance!

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.

5 participants