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

change to new cython version #839

Merged
merged 4 commits into from
Jul 26, 2018

Conversation

wkerzendorf
Copy link
Member

@wkerzendorf wkerzendorf commented Jul 18, 2018

@unoebauer, @scoder
version 0.28.4 should have the right fix in there.

cython/cython#2199

@wkerzendorf
Copy link
Member Author

@unoebauer - should we merge?

@unoebauer
Copy link
Contributor

I think we should - Travis is happy with both Linux and OSX builds, so I don't see a reason why we shouldn't. We might wait for @Heringer-Epson though to check whether this version works on his local installation.

@wkerzendorf
Copy link
Member Author

@unoebauer - no I removed the OSX build for now.

@wkerzendorf
Copy link
Member Author

I would merge and put a reminder in my phone to check in 3 months to reinclude the OSX build

@unoebauer
Copy link
Contributor

Sorry, I missed that the OSX is deactivated. So on OSX the segfault is still triggered? And locally on your OSX build it works?

@wkerzendorf
Copy link
Member Author

@unoebauer it doesn't segfault in the debugger even. I think, I will just disable it for now.

@Heringer-Epson
Copy link
Contributor

Sorry for the delay. I will check this today.

@Heringer-Epson
Copy link
Contributor

I tried to install the current tardis version with Cython 0.28.4. Still got a segmentation fault.

To be clear, I was having trouble checking out the PR 59e625f, so I simply modified the tardis_env27.yml file to uncomment Cython and have it the 0.28.4 version. When I tried to python setup.py install, I got the following error:

tardis/simulation/setup_package.py:2: RuntimeWarning: Parent module 'tardis.simulation' not found while handling absolute import
from setuptools import Extension
tardis/simulation/setup_package.py:3: RuntimeWarning: Parent module 'tardis.simulation' not found while handling absolute import
from astropy_helpers.setup_helpers import get_distutils_option
tardis/simulation/setup_package.py:4: RuntimeWarning: Parent module 'tardis.simulation' not found while handling absolute import
import numpy as np
tardis/plasma/setup_package.py:2: RuntimeWarning: Parent module 'tardis.plasma' not found while handling absolute import
from setuptools import Extension
tardis/plasma/setup_package.py:3: RuntimeWarning: Parent module 'tardis.plasma' not found while handling absolute import
from astropy_helpers.setup_helpers import get_distutils_option
tardis/plasma/setup_package.py:4: RuntimeWarning: Parent module 'tardis.plasma' not found while handling absolute import
import numpy as np
tardis/montecarlo/setup_package.py:2: RuntimeWarning: Parent module 'tardis.montecarlo' not found while handling absolute import
from setuptools import Extension
tardis/montecarlo/setup_package.py:3: RuntimeWarning: Parent module 'tardis.montecarlo' not found while handling absolute import
import numpy as np
tardis/montecarlo/setup_package.py:4: RuntimeWarning: Parent module 'tardis.montecarlo' not found while handling absolute import
import os
tardis/montecarlo/setup_package.py:5: RuntimeWarning: Parent module 'tardis.montecarlo' not found while handling absolute import
from astropy_helpers.setup_helpers import get_distutils_option
tardis/montecarlo/setup_package.py:6: RuntimeWarning: Parent module 'tardis.montecarlo' not found while handling absolute import
from Cython.Build import cythonize
tardis/montecarlo/setup_package.py:8: RuntimeWarning: Parent module 'tardis.montecarlo' not found while handling absolute import
from glob import glob
Freezing version number to tardis/version.py
Compiling tardis/montecarlo/montecarlo.pyx because it changed.
[1/1] Cythonizing tardis/montecarlo/montecarlo.pyx
Segmentation fault (core dumped)

@wkerzendorf
Copy link
Member Author

@Heringer-Epson be sure to delete montecarlo.c (look at it - it will say generated with Cython VERSION_NUMBER then rerun the compilation.

Copy link
Contributor

@unoebauer unoebauer left a comment

Choose a reason for hiding this comment

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

We should merge after we hear back from @Heringer-Epson

@@ -87,15 +88,9 @@ install:
- cd $TRAVIS_BUILD_DIR
- conda env create -f tardis_env27.yml
- source activate tardis
#trouble with building due to segfault at cython (https://github.com/cython/cython/issues/2199)
#remove if we can get normal cython through conda
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we leave this in as a comment? This way, we'll know what to do when some strange cython error appears in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

That’s why I put it in

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of keeping the procedure of getting bleeding-edge cython via github and compiling it with custom flags as a comment block and providing some additional information.

@Heringer-Epson
Copy link
Contributor

Yes, this worked upon trying to rerun the installation. Note, using the conda env suggestion in tardis_env27.yml triggers an error due to Pyqt not being installed (I believe it's required for the gui interface). Installing Pyqt=4.11 on the local env then caused a bunch of packages to be downgraded.

@wkerzendorf
Copy link
Member Author

that's interesting. anyways. I'll merge once I add the manual cython installing instructions in.

Copy link
Contributor

@unoebauer unoebauer left a comment

Choose a reason for hiding this comment

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

Excellent, exactly what I had in mind

@wkerzendorf wkerzendorf merged commit e7310e6 into tardis-sn:master Jul 26, 2018
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

3 participants