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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a notice of AVX2 to install by pip command #241

Merged
merged 2 commits into from
Oct 30, 2020

Conversation

y-yu
Copy link
Contributor

@y-yu y-yu commented Jul 8, 2020

  • Before merge Build sdist, bdist_wheel on CI and publish them to PyPI聽#233, we always install Qulacs from the source code when run pip install
  • However after that, the compiled Qulacs binary has been published to PyPI and download it
  • Qulacs binary in PyPI requires AVX2 but if the computer where the program using Qulacs will run doesn't support AVX2 then the program will almost certainly fail due to segmentation fault or something else 馃槆
  • So I add this notice for the user who want to install Qulacs by pip

Copy link
Contributor

@kwkbtr kwkbtr left a comment

Choose a reason for hiding this comment

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

Thanks!

README.md Outdated Show resolved Hide resolved
Co-authored-by: KAWAKUBO Toru <dev@kwkbtr.info>
@leofang
Copy link

leofang commented Aug 6, 2020

Hi @y-yu Thanks for discovering this issue. @ycchen1989 is also on an old Sandybridge machine without AVX2, and when we test the Conda package we build we keep running into core dump without any clue, until we see this PR.

It turns out that when we do cross-compiling, we need to pass -DUSE_SIMD=No and -DOPT_FLAGS=-march=sandybridge ... to cmake for targeting Sandybridge, see how we fix it in our Conda recipe: https://github.com/nsls-ii-forge/qulacs-feedstock/blob/b8afd0011e9412703800b1c69f8b85eff00cffd5/recipe/meta.yaml#L81-L84
So while adding a warning to README is certainly a good move, perhaps it's even better to address this issue at the cmake level? I am not sure if PyPI supports different versions of wheels for different CPU archs, though.

@kwkbtr
Copy link
Contributor

kwkbtr commented Aug 6, 2020

@leofang @ycchen1989 Sorry for your inconvenience.

It turns out that when we do cross-compiling, we need to pass -DUSE_SIMD=No and -DOPT_FLAGS=-march=sandybridge ... to cmake for targeting Sandybridge, see how we fix it in our Conda recipe: https://github.com/nsls-ii-forge/qulacs-feedstock/blob/b8afd0011e9412703800b1c69f8b85eff00cffd5/recipe/meta.yaml#L81-L84

Yes, -DUSE_SIMD=No is necessary since USE_SIMD means using AVX2 in Qulacs.

So while adding a warning to README is certainly a good move, perhaps it's even better to address this issue at the cmake level?

Could you elaborate on "address this issue at the cmake level"?
The current CMakeLists.txt has a default OPT_FLAGS, but it can be overridden by QULACS_OPT_FLAGS environment variable just as you have done in your link.
If you are suggesting adding this instruction to README, I agree that it would be helpful for users.

I am not sure if PyPI supports different versions of wheels for different CPU archs, though.

In my understanding it seems impossible, that's why we only publish wheels with AVX2 enabled.

@leofang
Copy link

leofang commented Aug 6, 2020

Yes, -DUSE_SIMD=No is necessary since USE_SIMD means using AVX2 in Qulacs.

Could you elaborate on "address this issue at the cmake level"?
The current CMakeLists.txt has a default OPT_FLAGS, but it can be overridden by QULACS_OPT_FLAGS environment variable just as you have done in your link.

My point is that setting USE_SIMD=No is not sufficient when cross-compiling. But now that I think about it, it seems to be hard to have some kind of auto configure to fix this. (Was thinking a ./configure ...; make; make install approach to discover the architecture features, but it doesn't seem to help...)

If you are suggesting adding this instruction to README, I agree that it would be helpful for users.

Only for users who are cross-compiling (like us 馃槀).

In my understanding it seems impossible, that's why we only publish wheels with AVX2 enabled.

Yep. Right now on NSLS-II-Forge we build three variants:

conda install -c nsls2forge -c conda-forge qulacs                        # cpu, with simd
conda install -c nsls2forge -c conda-forge qulacs=*=*no_simd             # cpu, without simd
conda install -c nsls2forge -c conda-forge qulacs-gpu [cudatoolkit=X.Y]  # gpu, X.Y = 9.2, 10.0, 10.1, 10.2; don't think simd matters?

If you're interested in seeing this appear on Conda-Forge, please open an issue on https://github.com/nsls-ii-forge/qulacs-feedstock/ so that we can discuss 馃檪 The migration should be fairly straightforward.

@kwkbtr
Copy link
Contributor

kwkbtr commented Aug 6, 2020

My point is that setting USE_SIMD=No is not sufficient when cross-compiling. But now that I think about it, it seems to be hard to have some kind of auto configure to fix this. (Was thinking a ./configure ...; make; make install approach to discover the architecture features, but it doesn't seem to help...)

Yes, the current default value -march=native ... produces binary optimized for the machine on which the compilation is performed. I suppose you need to specify architecture features explicitly for corss-compiling. ./configure may discover architecture of the current machine (which is accomplished by -march=native for some extent), but the produced binary would not run on machines with different architecture.

Yep. Right now on NSLS-II-Forge we build three variants:

conda install -c nsls2forge -c conda-forge qulacs                        # cpu, with simd
conda install -c nsls2forge -c conda-forge qulacs=*=*no_simd             # cpu, without simd
conda install -c nsls2forge -c conda-forge qulacs-gpu [cudatoolkit=X.Y]  # gpu, X.Y = 9.2, 10.0, 10.1, 10.2; don't think simd matters?

If you're interested in seeing this appear on Conda-Forge, please open an issue on https://github.com/nsls-ii-forge/qulacs-feedstock/ so that we can discuss 馃檪 The migration should be fairly straightforward.

Indeed, we may be able to publish to PyPI several packages with different names, for example qulacs and qulacs-no-simd.

@corryvrequan corryvrequan merged commit 7c08283 into qulacs:dev Oct 30, 2020
@y-yu y-yu deleted the add-readme-avx2 branch November 29, 2020 12:49
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.

None yet

4 participants