-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Bug fixes] fix config attributes backward compatibility #4237
Conversation
Thanks for your contribution! |
def __getattribute__(self, name: str): | ||
if not self.constructed_from_pretrained_config(): | ||
raise AttributeError(f"'{type(self)}' object has no attribute '{name}'") | ||
|
||
# FIXME(wj-Mcat): for details, please refer to: https://github.com/PaddlePaddle/PaddleNLP/pull/4201#discussion_r1057063402 | ||
# this condition branch code will be removed later. | ||
if name in self.config.attribute_map: | ||
logger.warning(f"do not access config from `model.{name}`, you should use: `model.config.{name}`") | ||
return self.config[name] | ||
|
||
if name in self.config.standard_config_map: | ||
logger.warning(f"do not access config from `model.{name}`, you should use: `model.config.{name}`") | ||
return self.config[name] | ||
|
||
return super().__getattribute__(name) | ||
|
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.
这里通过__getattr__
和 __getattribute__
都存在循环属性获取的问题,理论上__getattr__
只有属性不存在的时候才会进入这个函数,可是我在测试的时候,即使属性存在也会进入到该函数当中。比较奇怪。
测试代码如下:
from paddlenlp.transformers import BertConfig, BertModel
config = BertConfig.from_pretrained('__internal_testing__/bert')
model = BertModel(config)
a = model.embeddings
try: | ||
return super(PretrainedModel, self).__getattr__(name) | ||
except AttributeError: | ||
result = getattr(self.config, name) |
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.
所以说num_classes -> num_labels这种attribute map就交给底层的PretrainedConfig来支持了是吧?
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.
这里的代码逻辑是:如果 model 获取不到,就从 config 获取,并且抛出warning。
model = ErnieModel(config) | ||
model.eval() | ||
assert model.num_classes == config.num_labels |
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.
能不能写进common_test里,对于各种model_class (e.g. ErnieModel, ErnieForQuestionAnwering), 所有在attribute_map和standard_map里的都被正确的映射。然后配置一个一键turn-on 测试
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.
可以。
Codecov Report
@@ Coverage Diff @@
## develop #4237 +/- ##
===========================================
+ Coverage 35.08% 35.98% +0.90%
===========================================
Files 408 419 +11
Lines 57029 59060 +2031
===========================================
+ Hits 20007 21254 +1247
- Misses 37022 37806 +784
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
lgtm
PR types
Bug fixes
PR changes
APIs
Description
解决 config 属性字段兼容问题