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

Wheel distribution compiles cython files with -O2 despite long compil… #798

Merged
merged 3 commits into from
Nov 4, 2020

Conversation

nrnhines
Copy link
Member

…e time.

Closes #518

setup.py Outdated
extra_compile_args=["-O0"], # cython files take too long to compile with O3
# Cython files take a long time to compile with O2 but this
# is a distribution...
extra_compile_args=["-O2"],
Copy link
Member

Choose a reason for hiding this comment

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

this gets also used during pull requests which will delay the build. Should add CLI flag to setup.py where we can indicate release build or developer build?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. Our sanity depends on doing this only for Azure wheel builds that become neuron-nightly or neuron .whl distributions. I wanted to look at the build times for this and compare to one of the builds without this change.
Can you point me to the place or environment variable name that indicates this is going to be a published distribution?

Copy link
Member Author

Choose a reason for hiding this comment

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

It appears that the hit is not too bad for the linux wheels on azure eg.

python35 4m 56s 5m 37s
python36 4m 29s 6m 28s
python37 4m 44s 6m 39s
python38 5m 14s 5m 33s
but mac wheels are quite a bit worse
python35 8m 10s 15m 36s
python36 7m 40s 14m 20s
python37 10m 47s 16m 51s
python38 7m 52s 14m 16s

clearly we want to use the -O2 only for distributions.

Copy link
Member

Choose a reason for hiding this comment

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

Test wheel upload is decided by this line:

condition: and(succeeded(), in(variables['Build.Reason'], 'Manual', 'Schedule'), ne(variables['NRN_NIGHTLY_UPLOAD'], 'false'))

which means if the build is nightly scheduled or if someone manually trigger build but NRN_NIGHTLY_UPLOAD !=False then test wheel is uploaded. (Note : now days we are explicitly triggering CI plans too often. We should remove upload on manual launch)

Release uploaded with this line :

condition: and(succeeded(), in(variables['Build.Reason'], 'Manual'), eq(variables['NRN_RELEASE_UPLOAD'], 'true'))

i.e. one has trigger build by explicitly setting env variable NRN_RELEASE_UPLOAD=True (azure CI web interface has option to setup env variable)

Copy link
Member

@alexsavulescu alexsavulescu Oct 28, 2020

Choose a reason for hiding this comment

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

Adding a CLI flag sounds like a good idea. Another solution that comes to mind would be using en ENV var that would be set only on Azure.

Btw, RX3D is set to OFF, maybe setting that to ON adds up to the build time?

Copy link
Member Author

Choose a reason for hiding this comment

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

RX3D is set to OFF

Is that specified in the travis somehow? Setup.py does not do this through cmake

            '-DNRN_ENABLE_RX3D=OFF',  # Never build within CMake

but with

if '--disable-rx3d' in sys.argv:
...
else:
    RX3D = True

and packaging/python/build_wheels.bash does not pass the --disable-rx3d arg to setup.py

I actually don't know why rx3d is handled specially in setup.py and not with the usual cmake args.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't get to look into it but I suspect that if-else doesn't affect cmake. From Azure logs we can see:

-- RX3D          | OFF

Copy link
Member Author

Choose a reason for hiding this comment

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

I notice that win_build_cmake.sh also always uses -DNRN_RX3D_OPT_LEVEL=2 . Is this a travis test also, or is it only
done by azure for a published distribution?

Copy link
Member

Choose a reason for hiding this comment

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

Looking at what we have, I wonder why we have two Travis jobs building the wheel, since we already do it on Azure. I get the feeling they are redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

-- RX3D | OFF
That is just the cmake status. The nrn/setup.py has its own elaborate explicit RX3D builld commands. Don't know why that was done. @ferdonline Why did nrn/setup.py need

'-DNRN_ENABLE_RX3D=OFF',  # Never build within CMake

Copy link
Member

@alexsavulescu alexsavulescu left a comment

Choose a reason for hiding this comment

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

Very nice approach

@nrnhines nrnhines merged commit 8a0c5e3 into master Nov 4, 2020
@nrnhines nrnhines deleted the wheel-rx3d-opt2 branch November 4, 2020 16:59
@nrnhines nrnhines restored the wheel-rx3d-opt2 branch November 4, 2020 17:00
@nrnhines nrnhines deleted the wheel-rx3d-opt2 branch November 4, 2020 17:00
olupton pushed a commit that referenced this pull request Dec 7, 2022
* The getter and setter methods are bound to a pybind11 property.
* Add unit tests for parent property
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.

All release build scripts should use -DNRN_RX3D_OPT_LEVEL_DEFAULT "2"
3 participants