-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Pyqstem #8656
Pyqstem #8656
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
recipes/pyqstem/meta.yaml
Outdated
- fftw | ||
- numpy | ||
- python | ||
- setuptools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numpy, python, fftw, and setuptools should be in host
, not build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will try that - hopefully it will make it build correctly :-)
- fftw | ||
- numpy | ||
- python | ||
- pip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YOu can also move cython to host if that helps
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/pyqstem:
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
Mayday! I have a problem building this package. On some architectures (Windows), it fails to use the FFTW package apparently due to some problem with a prerequisite (MPI of some kind). The package does not use mpi itself. Anybody knows how to solve this? @scopatz ?
|
I am not sure about this one. Cc @conda-forge/staged-recipes @conda-forge/fftw @conda-forge/mpi @conda-forge/mpich |
A maintainer of gmt-feedstock (@leouieda) also recently reported this same problem (their recipe is trying to pull in an MPI-based FFTW variant on Windows when MPI was not requested). (see conda-forge/fftw-feedstock#53) ping @beckermr: As the contributor of the MPI builds to FFTW, do you have any idea what could be going wrong here? |
I have explicitly checked the FFTW windows package in question and it has no MPI requirement. Thus the MPI dependence must be coming from some other place. I don't see it in our global pinning files. Maybe conda build added an internal default? Maybe the solver has a new bug? TBH I am not sure. |
Looking at the linux build is giving us a hint here. Here is the test env
Notice it has MPI. Thus some runtime dependence of the package is generating an MPI depence in some way. I suggest the recipe authors look through the various dependencies, dependencies of dependencies etc to figure out which one it is. If the dependence is forcing MPI when it should not be, we may need to add the MPI selector stuff used elsewhere in conda-forge to that dependence. |
@beckermr @scopatz @schiotz I was just about to post to gitter asking about this. This change must be recent because this problem didn't happen last week. That might help narrow down the list. We probably share the dependency between this recipe and https://github.com/conda-forge/gmt-feedstock |
I just created an environment with the packages above and ran the following on Jupyter for all packages listed by
Grepping for |
Looks like it is something either in the solver or the metadata for windows then. Glad we are narrowing things down! |
@beckermr it might be something different in the solver. Conda just released 4.7 this week but the fftw build from 2 months ago was causing trouble. |
It is probably related to conda/conda#8844 (comment) |
Thanks everyone for getting involved in this - what a great community! It looks like I just have to wait for the dust to settle after the last upgrade, then eventually this will just work. :-) |
Still broken 😦 |
This latest build still ran with conda 4.7.5. I think we need to wait until 4.7.6 maybe. |
I don't think new conda is going to fix anything for you. We're not bringing the free channel back. First, you need to isolate whether using the free channel fixes things for you:
If so, you next need to identify the package(s) from free that you are depending on. Finally, you need to work with the package maintainers for those packages to rebuild them so that they don't depend on the free channel. The only thing that new conda might help is better hints that make it easier to figure out what needs to be rebuilt. |
OTOH all of the packages in the env were checked for mpi above and none had a dependence. What is the mechanism by which the free channel fixes things? |
The dependence on mpi is a red herring. When the solver can't achieve what you want, it goes and finds solutions that may seem really strange. Unsatisfiability should be your first suspect in these situations. Try it with the free channel re-enabled. If it works, that's definitely the culprit, and the hard part is then figuring out which packages from free are necessary. If you add |
Alright. The developers of the recipe should try the suggestions above and report back. |
@beckermr @msarahan I assume I need to set three channels: default, free and conda-forge with the latter having highest priority. Should I set strict channel priority? I am currently reinstalling VS 2015 on the Windows server where I can play around with this. I cannot test it on my Mac, where things are just working. |
@beckermr @msarahan
|
Wow. I’m baffled. Maybe we should debug the Linux build first. It also depends on MPI for some reason at runtime. |
Linux support is generally better throughout the ecosystem so it might be easier for everyone to contribute to that. |
Would it make sense to split this recipe into two: "pyqstem" and "qstem" in order to be able to pull the qstem source from its source (https://github.com/QSTEM/QSTEM)? |
The fftw mpi build is due to a bug in conda-build >= 3.18.4. |
Thanks @minrk! |
I don't think so, it would just complicate matters. The pyqstem developer (@jacobjma) decided to bundle the relevant parts of the QSTEM source code with pyqstem, and I do not think we should change that without a good reason. |
Linux would be an order of magnitude easier for me, since I would actually know what I was doing :-) |
@schlotz: I think the issue on linux that @beckermr is referring to is the following: If you look at the linux azure log At build time we see a
but then later in the log at run time (the testing phase) we see MPI getting pulled in when it should not be:
This doesn't fail like on Windows because MPI-based packages exist on linux, but this should not be happening. Going back to an older conda-build (<3.18.4) may resolve the issue, but I don't think anyone has verified that yet. |
@grlee77 Thank you for your clear explanation. It looks like the only thing I can do is to wait until the wizards are done :-) Edit: grammar |
One concern here is that pyqstem is python wrapper for QSTEM and is vendoring QSTEM, which means that QSTEM does not come from the official source. One none negligible issue is that it is not clear what version of QSTEM is included in pyqstem. |
@ericpre |
@schlotz, it looks like the recent conda-build versions that were causing problems are now marked as broken, so this might work without the MPI issues if you want to try again. |
@grlee77 Thank you very much! And it looks like somebody triggered a rebuild - now it works! |
Thanks all for the help here. Merging. |
I triggered the builds. Thanks a bunch everyone! |
Note: This is my very first recipe, and the code uses C/C++. I may very well have done something wrong, any feedback is very welcome.
Checklist