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

[Fix] Release resources after use #133

Merged
merged 11 commits into from
Dec 22, 2023

Conversation

Bobholamovic
Copy link
Member

@Bobholamovic Bobholamovic commented Dec 7, 2023

本PR主要解决erniebot/http_client.py中的两个资源管理相关问题:

  1. 目前erniebot库主要提供函数式接口,但出于性能考虑对requests session进行了缓存,这导致资源泄漏(无用的session在内存中长期存在)。本PR重新设计了erniebot库的session部分:当用户未进行任何额外配置时,默认为每个request新建一个session,此时调用方式简单但性能较差,适合用于做demo;当用户配置requests_sessionaiohttp_session时,支持复用requests/aiohttp会话,可以在多个EBClient对象乃至全局层面共享会话,从而提升性能。该设计的一些选择和主要考虑如下:

    • 默认为每个request新建一个session是因为对于函数式接口,我们没有安全的办法维护persistent session,特别是考虑到异步IO的情况。
    • EBClient不管理session资源的申请和释放,也就是说它view而不是own session。这主要是出于如下考虑:
      • 我们目前对外主要支持的是函数式接口(面向对象的接口虽然可用但没有文档),函数式接口的好处是简单、易于理解、符合人类直觉,坏处是无法利用OOP的好处。在以函数式接口为优先的前提下,viewing session的设计可以为用户带来循序渐进的开发体验。一开始,用户跑通demo,完全不在意性能,也对每个request新建一个session这样的细节不了解;后来,可能发现应用有了提升性能的需求,这时候如果应用中只涉及到一个backend的一类资源(根据用户反馈,这是很常见的使用场景),那么就只需要在原本代码的基础上增设一个全局session;最后,如果应用足够复杂,需要更精细化的资源控制,可以考虑在调用接口时配置_config_。而如果我们让EBClient own session,局部的session变得相对没意义(毕竟每次传入的session只会用在一次请求),全局session的情况就会变得复杂(我们需要确保EBClient对全局和局部的session的处理逻辑不一样,全局session不会在一次请求后就被销毁)。虽然在函数式接口以外推广一套面向对象的接口以自动管理session也是一个可选项,但就项目目前的情况而言,这种做法的开发成本较高,对现存用户代码的影响面也较大。
      • 我们支持配置resource对象级别(也就是EBClient对象级别,因为每个resource对象持有一个client)session的目的主要是在不同的request之间共享session,而这些session实际上也是可以在更大的范围内共享的,例如全局共享或是一个backend的不同resource之间共享(我们假设应用中每次可能更倾向于只用到一种resource,因此每个resource都是单独的类,采用单resource可选多backend的设计而不是单client集成多resource的设计),EBClient对象中不释放资源有助于开发者在外部进行更精细化的资源共享配置,只是需要开发者自行进行资源的申请和释放。可以说,viewing object更适配当前的resource类设计。
  2. 完成HTTP请求后没有及时关闭连接,在并发较高的情况下,这可能导致连接池资源耗尽。本PR修复了这个问题。

@sijunhe
Copy link
Collaborator

sijunhe commented Dec 8, 2023

@juncaipeng 看吧,这个PR我没有context

@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eb4069d) 61.83% compared to head (99a1de4) 61.45%.
Report is 6 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #133      +/-   ##
===========================================
- Coverage    61.83%   61.45%   -0.38%     
===========================================
  Files           56       56              
  Lines         2780     2771       -9     
===========================================
- Hits          1719     1703      -16     
- Misses        1061     1068       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Bobholamovic Bobholamovic marked this pull request as draft December 21, 2023 18:55
@Bobholamovic Bobholamovic marked this pull request as ready for review December 22, 2023 08:39
@juncaipeng juncaipeng merged commit 7fba798 into PaddlePaddle:develop Dec 22, 2023
5 of 6 checks passed
@Bobholamovic Bobholamovic deleted the fix/release_resource branch December 22, 2023 10:50
skyan pushed a commit to skyan/ERNIE-Bot-SDK that referenced this pull request Dec 26, 2023
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