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

optimize setup.py under conda virtual environment #48211

Merged
merged 3 commits into from
Feb 13, 2023

Conversation

risemeup1
Copy link
Contributor

@risemeup1 risemeup1 commented Nov 21, 2022

PR types

others

PR changes

others

Describe

优化setup.py
当我们通过conda 创建一个新的空的环境时候,setup.py不会检查依赖,直接编译,会在cmake阶段报错缺少依赖,现在在cmake之前进行判断,并提示用户安装依赖,如缺少protobuf,则会提示,如图
image

@paddle-bot
Copy link

paddle-bot bot commented Nov 21, 2022

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@risemeup1 risemeup1 changed the title add eager and jit directory coverage rate optimize setup.py under conda virtual environment Feb 9, 2023
setup.py Outdated
@@ -1405,10 +1410,31 @@ def get_setup_parameters():
)


def check_build_dependency():
python_dependcies_module = [
Copy link
Contributor

Choose a reason for hiding this comment

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

这个列表直接从python/requirements.txt中获取吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

对,完全一致

Copy link
Contributor Author

Choose a reason for hiding this comment

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

对,和里边完全一致

Copy link
Contributor

Choose a reason for hiding this comment

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

尽量不要维护两处一模一样的内容,直接从requirements.txt中读取

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,很好的建议

installed_packages = [r.decode().split('==')[0] for r in reqs.split()]
for dependency in python_dependcies_module:
if dependency not in installed_packages:
raise RuntimeError(missing_modules.format(dependency=dependency))
Copy link
Contributor

Choose a reason for hiding this comment

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

是否可以一次暴露出所有缺失模块

Copy link
Contributor Author

Choose a reason for hiding this comment

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

可以,我优化一下,但是最后提示阶段,还是提示用户pip install -r python/requirements,而不是提示只安装缺失的包,因为python依赖库都有版本限制,pip install -r python/requirements可以确保依赖库版本都正确

Copy link
Contributor

Choose a reason for hiding this comment

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

是的,提示出缺少哪些包

setup.py Outdated
@@ -76,6 +76,11 @@

IS_WINDOWS = os.name == 'nt'

missing_modules = '''
Copy link
Contributor

Choose a reason for hiding this comment

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

不需要单独维护一个global string,直接写在函数中就好

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,我改一下

f.read().splitlines()
) # Specify the dependencies to install

python_dependcies_module = []
Copy link
Contributor

Choose a reason for hiding this comment

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

利用正则会更简单一些,例如:re.sub("_|-", '', re.sub(r"==.|>=.|<=.*", '', "abc_d-e<=1.1")) 返回 abcde

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Copy link
Contributor

@zhangbo9674 zhangbo9674 left a comment

Choose a reason for hiding this comment

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

LGTM

@risemeup1 risemeup1 merged commit 5143b0e into PaddlePaddle:develop Feb 13, 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.

2 participants