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

Configure RTD #161

Merged
merged 3 commits into from
Apr 20, 2017
Merged

Configure RTD #161

merged 3 commits into from
Apr 20, 2017

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Apr 4, 2017

Builds the docs for pyFFTW on ReadTheDocs. Requires a hack to fix use the same toolchain that ReadTheDocs uses. Also relies on PR ( #162 ) to set some reasonable search paths for use with conda packages and PR ( #164 ) to fix some things with the Sphinx configuration.

ref: http://jakirkhampyfftw.readthedocs.io/en/config_rtd/index.html
ref: https://readthedocs.org/projects/jakirkhampyfftw/builds/5245737

cc @hgomersall @grlee77

@jakirkham jakirkham mentioned this pull request Apr 4, 2017
@jakirkham jakirkham force-pushed the config_rtd branch 7 times, most recently from ae75dbb to 1fd0bdd Compare April 4, 2017 03:41
@jakirkham
Copy link
Member Author

Tried restructuring the docs, but that doesn't seem to have fixed it. It seems like there is still something off. FWICT it seems to be related to the import structure somehow. 😕

@jakirkham jakirkham force-pushed the config_rtd branch 5 times, most recently from 578d464 to 32ae1c1 Compare April 4, 2017 14:36
@jakirkham jakirkham changed the title WIP: Configure RTD Configure RTD Apr 4, 2017
@jakirkham
Copy link
Member Author

🎉 This now works! 🎉

Please take a look and share feedback.

@grlee77
Copy link
Contributor

grlee77 commented Apr 4, 2017

great to see this is now working!

I was not previously familiar with the readthedocs.yml for specifying conda packages. That is pretty nice!

It's not ideal that we have to hack setup.py to find gcc, but I don't see that it should cause any problems. I guess an alternative hack would be to try adding the gcc package from conda-forge in the environment.yml and see if that one gets picked up by default? (at cost of slightly longer build times do to the additional gcc download).

@jakirkham
Copy link
Member Author

Thanks for the feedback, @grlee77.

Yeah, conda support is relatively new. It had been in the works for a while, but got completed last year. For a bit of history, please see issue ( readthedocs/readthedocs.org#857 ).

Using gcc is a good idea. I tried this initially, but it didn't work. The gcc package in use has some post link script, which failed in the RTD build environment. This post link script expects to be able to find out some information about the environment and makes a lot of assumption based on a variety of Linux distros tried. However it just is unable to do this for the RTD build environment for whatever reason. I tried rolling back to earlier versions of the gcc package including one without this post link script, but that fails due to the short prefix. Given these issues, I decided trying to change the compiler used in setup.py for RTD was the least bad option. Still not sure why it is necessary for pyFFTW, but not necessary for PyWavelets.

@hgomersall
Copy link
Collaborator

I'll have to look over this later, I need to run now.

Are there any potential issues with these changes? Or is the net effect that the RTD docs are up to date?

@jakirkham
Copy link
Member Author

It has PR ( #164 ) in the history. So might want to check if you are ok with that change.

Otherwise, no, it just sets up RTD to build.

Seems like this is confusing Sphinx in terms of where to look for the
Python sources. So if we simply drop this manipulation, this should no
longer cause a problem.
Relying on the `setup.py` file to get the version information doesn't
seem like a good idea. It also requires `pyfftw` to be built in source
to build the docs, which really shouldn't be a requirement. Given
`pyfftw.version` will have this information and should be created in
source or out-of-source builds, use that instead to extract the version
information.
Provides the necessary configuration files and settings to build on
ReadTheDocs. Requires a hack to the build system to handle the toolchain
configuration in the ReadTheDocs environment. Otherwise everything else
is pretty standard.
@hgomersall
Copy link
Collaborator

Apologies for the delay on this, I've been very busy.

@jakirkham jakirkham deleted the config_rtd branch April 20, 2017 13:33
@jakirkham
Copy link
Member Author

No worries. I think the RTD project settings might need to be refreshed though. Unfortunately there is not much I can do on from my end.

@hgomersall
Copy link
Collaborator

What needs to be done? When you say not much can be done is that because I need to do it?

@jakirkham
Copy link
Member Author

Yes. It's been a little while since I did this, but I would first check to see if the RTD service is enabled on this repo. Maybe it was disabled after the initial setbacks, which makes sense.

I tried to put everything else in the readthedocs.yml file in this PR. So I don't think there are any setting changes. However if it doesn't work after enabling RTD and doing a fresh rebuild, please ping me and I can look closer.

@hgomersall
Copy link
Collaborator

Ok will do, thanks. btw, I invited you to be a maintainer. It's about time I excited other people in pyFFTW and you've been keen enough so far!

@jakirkham
Copy link
Member Author

So I tried creating a fresh RTD project without any setting changes and that seemed to work. It built a copy of docs under latest correctly. stable was not correct, but I think that uses the latest tag instead of master, which is what latest does. Guessing the only thing that needs to be updated is the webhook then.

@jakirkham
Copy link
Member Author

Cool, thanks. I plan to extend the Dask interface to 2-D and N-D on the next release of Dask.

We like using pyFFTW internally for image registration. It makes for a nice speedup beyond what NumPy' FFTPACK can do. Not to mention if it helps a convolution with a large kernel go a bit faster, that is nice too.

@hgomersall
Copy link
Collaborator

So I've set up the webhooks. I guess we see what happens...

For the time being, I'm happy with just latest working!

@hgomersall
Copy link
Collaborator

I really appreciate all your work on this, it's very valuable.

@jakirkham
Copy link
Member Author

Great, thanks. No problem. FWIW it looks like it is working.

FYI may need to clear your cache if the docs aren't showing up correctly.

@hgomersall
Copy link
Collaborator

No, it looks fine :)

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