Skip to content

Update c++17 for pytorch 2.1.0#101

Open
yanbing-j wants to merge 2 commits into
HawkAaron:masterfrom
yanbing-j:yanbing/update_cxx17
Open

Update c++17 for pytorch 2.1.0#101
yanbing-j wants to merge 2 commits into
HawkAaron:masterfrom
yanbing-j:yanbing/update_cxx17

Conversation

@yanbing-j
Copy link
Copy Markdown

@yanbing-j yanbing-j commented Jun 6, 2023

This PR is to update C++17 for PyTorch 2.1.0.

@yanbing-j
Copy link
Copy Markdown
Author

Hi @HawkAaron @PoDuReM @IsaacTay @kamo-naoyuki, could you please help review this PR? Thank you!

Comment thread pytorch_binding/setup.py


extra_compile_args = ['-fPIC']
if LooseVersion(torch.__version__) >= LooseVersion("1.5.0"):
Copy link
Copy Markdown
Contributor

@PoDuReM PoDuReM Jun 6, 2023

Choose a reason for hiding this comment

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

What's the reason to use few c++ version? Why we can not use c++17 for any PyTorch version?

Copy link
Copy Markdown
Author

@yanbing-j yanbing-j Jun 7, 2023

Choose a reason for hiding this comment

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

PyTorch master has switched to C++17, need to update C++ version of this repo, otherwise, it will build fail. I have tested pytorch 1.10.0 and 1.13.0, C++17 works well. Suppose we can use C++17 for any PyTorch version.

Comment thread pytorch_binding/setup.py Outdated
extra_compile_args += ['-std=c++17']
else:
extra_compile_args += ['-std=c++11']
extra_compile_args += ['-std=c++14']
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about PyTorch versions less 1.5.0? Earlier c++11 used for these, now will be used c++14, is it ok?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

PyTorch version less 1.5.0 cannot run in my local platform. Since it is an old version, can we just use C++17 to cover all PyTorch version?

@yanbing-j yanbing-j requested a review from PoDuReM June 8, 2023 01:14
@yanbing-j
Copy link
Copy Markdown
Author

Hi @PoDuReM , could you please help review this PR? Thanks!

@PoDuReM
Copy link
Copy Markdown
Contributor

PoDuReM commented Jun 8, 2023

Hi @PoDuReM , could you please help review this PR? Thanks!

I'm satisfied with this merge request

@yanbing-j
Copy link
Copy Markdown
Author

yanbing-j commented Jun 8, 2023

Hi @PoDuReM , could you please help review this PR? Thanks!

I'm satisfied with this merge request

@PoDuReM Thank you so much! Can you help me merge into master as well?

@PoDuReM
Copy link
Copy Markdown
Contributor

PoDuReM commented Jun 8, 2023

Hi @PoDuReM , could you please help review this PR? Thanks!

I'm satisfied with this merge request

@PoDuReM Thank you so much! Can you help me merge into master as well?

I don't have enough permissions to merge into this repository

@yanbing-j
Copy link
Copy Markdown
Author

Hi @HawkAaron @IsaacTay @kamo-naoyuki, could you please help review this PR and merge it? Thank you!

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