-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[core
/TPLinear
] Fix breaking change
#1439
[core
/TPLinear
] Fix breaking change
#1439
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Younes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick fix. Only a small suggestions from my side, otherwise LGTM.
@@ -81,6 +81,14 @@ def __init__( | |||
|
|||
self.is_target_conv_1d_layer = False | |||
|
|||
@property | |||
def is_paralle_a(self): | |||
# See https://github.com/huggingface/peft/pull/1439 for more details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a # TODO remove in version X.Y
to these types of deprecations to make it easier to find them in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ! I propose to remove it in 2 minor releases
Thank you. I do not speak Python. |
* fix breaking change * add comment * add todo
Addresses: #1435 (comment)
Fixes a breaking change introduced by #1435
since
is_paralle_a
is a public attribute, it is a breaking change to completely remove it as some users might be using it alreadycc @pacman100 @BenjaminBossan