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

Alter faster to fast in FG #4213

Merged
merged 10 commits into from
Jan 1, 2023
Merged

Alter faster to fast in FG #4213

merged 10 commits into from
Jan 1, 2023

Conversation

FrostML
Copy link
Contributor

@FrostML FrostML commented Dec 23, 2022

PR types

Breaking changes

PR changes

Others

Description

Alter faster to fast in FG.

当前 PR 存在不兼容升级:

  • model.generate()use_faster 参数会修改成 use_fast
  • load("FasterTransformer") -> load("FastGeneration")

修复以下 BUG:

@paddle-bot
Copy link

paddle-bot bot commented Dec 23, 2022

Thanks for your contribution!

@codecov
Copy link

codecov bot commented Dec 23, 2022

Codecov Report

Merging #4213 (f5234fd) into develop (0399b72) will decrease coverage by 0.01%.
The diff coverage is 32.35%.

@@             Coverage Diff             @@
##           develop    #4213      +/-   ##
===========================================
- Coverage    36.32%   36.30%   -0.02%     
===========================================
  Files          419      419              
  Lines        59244    59223      -21     
===========================================
- Hits         21521    21503      -18     
+ Misses       37723    37720       -3     
Impacted Files Coverage Δ
paddlenlp/taskflow/code_generation.py 21.79% <ø> (ø)
paddlenlp/taskflow/text_summarization.py 11.65% <ø> (ø)
...lenlp/ops/fast_transformer/transformer/decoding.py 7.50% <4.87%> (ø)
paddlenlp/transformers/bart/modeling.py 85.30% <14.28%> (ø)
paddlenlp/transformers/gpt/modeling.py 77.25% <16.66%> (ø)
paddlenlp/transformers/codegen/modeling.py 89.03% <20.00%> (ø)
paddlenlp/transformers/mbart/modeling.py 81.76% <20.00%> (ø)
paddlenlp/transformers/pegasus/modeling.py 21.42% <20.00%> (ø)
paddlenlp/transformers/t5/modeling.py 85.28% <20.00%> (ø)
...lenlp/transformers/unified_transformer/modeling.py 82.06% <20.00%> (ø)
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sijunhe sijunhe marked this pull request as draft December 26, 2022 04:46
@sijunhe
Copy link
Collaborator

sijunhe commented Dec 26, 2022

convert to Draft since this is still under testing

@FrostML
Copy link
Contributor Author

FrostML commented Dec 26, 2022

Fix #4238 one by one...

@FrostML FrostML marked this pull request as ready for review December 27, 2022 06:49
@@ -541,7 +541,7 @@ def generate(
num_return_sequences=1,
diversity_rate=0.0,
use_cache=True,
use_faster=False,
use_fast=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

对之前的使用最好还是兼容一段时间并且给出deprecated的warning,之后再完全移除掉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

model.generate() 这里这么搞有点问题,我们的 repo 里面 log 打印的设置有点问题,如果设置全局的 warning filter,在任意 action 设置下,都不会有 warning 打印出来;如果在 def generate() 里面设置 filter,在任意设置下都会打印 warning,即,没法做到仅打印一次,而是每一次前向都会打印。

根据 generate() 预期的使用场景,这样会导致 log 爆炸。

Copy link
Contributor

@guoshengCS guoshengCS Dec 29, 2022

Choose a reason for hiding this comment

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

对于使用logging/logger时重复log的问题的避免 tokenizer_utils中有参考,增加flag通过flag来判断是否已经打印过

            if not self.deprecation_warnings.get("max_len_sentences_pair", False):
                logger.warning(
                    "Setting 'max_len_sentences_pair' is now deprecated. This value is automatically set up."
                )
            self.deprecation_warnings["max_len_sentences_pair"] = True

Copy link
Contributor

Choose a reason for hiding this comment

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

另外正常使用 warnings.warn 也会只打印一次,对于使用 warnings.warn 不打印的情况,还请 @wawltor @sijunhe 看看是否调整下咱们 Taskflow中的warnings的全局设置 https://github.com/PaddlePaddle/PaddleNLP/blob/develop/paddlenlp/taskflow/taskflow.py#L47

warnings.simplefilter(action="ignore", category=Warning, lineno=0, append=False)

当前的设置可能会对其他使用 warnings 的地方造成些干扰

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个simplefilter我不太清楚历史原因,还是由得@wawltor 解答

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已通过新增 self.deprecated_warnings 的方式控制打印的次数。

@sijunhe
Copy link
Collaborator

sijunhe commented Dec 29, 2022

cpp, cuda还有各种范例, doc里面直接faster -> fast, 我觉得是ok的. 在python端的model.generate()里面,是不是可以做一下向后兼容?例如

def generate(
  ...,
  use_fast,
  use_faster
  ...
):

if use_faster is not None:
  logger.warning(...)
  use_fast = use_faster
...

如果只在generate被调用的顶上做一次warning, 是不是就没有log爆炸的问题?

@FrostML
Copy link
Contributor Author

FrostML commented Dec 29, 2022

如果只在generate被调用的顶上做一次warning, 是不是就没有log爆炸的问题?

上面我的 comment 说的就是在 generate() 一开始调用 warnings.warn 出现的问题。可以换成 logger 试试。

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.

4 participants