-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Development Documentation Overhaul #10286
Conversation
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.
Some comments to help reviewers.
@@ -6,7 +8,7 @@ This document aims to give an overview of how to contribute to SciPy. It | |||
tries to answer commonly asked questions, and provide some insight into how the | |||
community process works in practice. Readers who are familiar with the SciPy | |||
community and are experienced Python coders may want to jump straight to the | |||
`git workflow`_ documentation. | |||
:ref:`contributor-toc`. |
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.
New contributors were not ready to jump to NumPy's git workflow documentation.
Now they are taken to a new Contributor Table of Contents (CTOC).
@@ -114,6 +116,29 @@ other users and yourself. Aiming for consensus in the discussion is important | |||
rare cases that agreement cannot be reached, the maintainers of the module | |||
in question can decide the issue. | |||
|
|||
.. _license-considerations: |
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.
Moved from general FAQ (which has been completely redistributed) to here, soon after it is needed. In this future, this could be written in a narrative style that matches the rest of the document.
@@ -194,289 +219,6 @@ project and SciPy the community, and it can always use a new pair of hands. | |||
The sources for the website live in their own separate repo: | |||
https://github.com/scipy/scipy.org | |||
|
|||
|
|||
Recommended development setup | |||
============================= |
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.
Moved to new recommended_development_setup.rst
linked from CTOC. Should probably be removed and included in individual build/install instructions, as the information in here is a useful part of setting up a development environment but is not nearly complete.
HACKING.rst.txt
Outdated
|
||
SciPy structure | ||
=============== | ||
|
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.
Moved to /doc/API.rst.txt
|
||
Checklist before submitting a PR | ||
-------------------------------- | ||
|
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.
Moved to Development Workflow
|
||
=========================== | ||
Running SciPy Tests Locally | ||
=========================== |
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.
New.
|
||
================= | ||
Git configuration | ||
================= |
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.
This and the rest of doc/source/dev/gitwash
adapted from NumPy documentation.
|
||
===================== | ||
Git for development | ||
===================== |
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 removed the Development Workflow documentation from this list (https://www.numpy.org/devdocs/dev/gitwash/index.html) so I followed suit. It feels sort of incomplete without it, though. Maybe will fill it out with the basic commands (like git commit
) at some point.
|
||
======== | ||
Git tips | ||
======== |
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.
Moved from Development Workflow.
doc/source/dev/index.rst
Outdated
|
||
========================== | ||
SciPy Core Developer Guide | ||
========================== |
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.
As a new contributor, I thought I should start with the "SciPy Developer Guide". Most of this isn't really appropriate for new contributors, though. Some of it could be useful, so it's linked from the CTOC, but hopefully now this looks less like it's supposed to be starting point for new contributors.
CI failure presumably for:
Will fix. |
It predates more extensive html docs, and that file name is (used to be) commonly used by projects for developer docs.
yes
don't, in the docs themselves. just put it in the description of this PR. that applies to other credits too (e.g. I saw a "thanks to Yarik ..." note somewhere).
I don't think you should link back to that. It's a very concise intro, and is a file that GitHub recognizes so it needs to be there. It's an intro point to link to the real docs, no need to reverse that flow of information/attention.
This naming is a bit odd. How about something like "Ways to contribute to SciPy" and "Contributor Guide"? |
Re: "Thanks to Yarik", I didn't add that. It was in the NumPy version, but I'll remove it. |
Changed names and fixed some errors, but somehow my changes are making Sphinx complain about 100 files not being included in any toctree. I compared to another doc build, and these warnings are the only messages not present there, so I assume they are why I will try adding all the new stuff to an invisible toctree to see if that helps. (Everything is already linked from Update: I had to add all my new files to a visible toctree to get Sphinx to stop complaining about them. Now the warnings are all about auto-generated files: |
I'm trying to improve the checklist for submitting a PR:
|
3f08ca4
to
51ecec1
Compare
@larsoner Hi Eric, you mentioned you'd be willing to review this. I added notes about finding a reviewer to Deciding on New Features, and I link to that in HACKING.rst.txt:
Before reviewing, I was wondering if you had any idea what is causing the Sphinx build to fail. My comment above:
describes the issue in more detail. I see there are also some new errors:
They just showed up when I moved the Core Developer documentation files into their own folder. I didn't make changes to all those files, and I don't see the duplicate labels it's complaining about. |
Should be able to look tomorrow. In the meantime can you rebase on latest |
4c679e7
to
5bec83e
Compare
@larsoner Genius! (or I'm a noob.) Now all that's left is the duplicate labels:
That seems to be the same as this, so perhaps I need to edit |
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.
Nearly all of the instructions were clear, and easy to follow. The accompanying videos were also tremendously helpful in the few places where the documentation was difficult to follow.
While I was unable to add our root SciPy directory to our PYTHONPATH, there appears to be a simpler solution than creating directories and editing files. The potential solution is detailed in the comments.
|
||
#. Enter ``conda install cython numpy matplotlib pytest spyder``. |br| Note that we're only installing SciPy's dependencies (and Spyder so we can use the IDE), but not SciPy itself. | ||
|
||
#. Our goal now is to add our root SciPy directory to the ``PYTHONPATH`` environment variable whenever this virtual environment is activated. This will ensure that Python can find the SciPy code we are trying to `import`. This requires adding a few files and folders deep inside your Anaconda installation directory. I suggest watching `the video <https://youtu.be/Faz29u5xIZc?t=35>`_ for this part. To summarize, you want to create: |
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.
I encountered some problems when adding our root development-SciPy directory to the PYTHONPATH environment variable.
First, the video tutorial differs from the written tutorial. In the video, the /conda/activate.d/env_vars.sh and /conda/deactivate.d/env_vars.sh were located within the /anaconda3/envs/scipydev/etc directory, not within the /anaconda3/envs/scipydev directory.
Second, trying either approach (with ~/env_vars.sh in either ~/scipydev or ~/scipydev/etc) failed to make our development-SciPy library available in Spyder. While the Conda installed SciPy library was available in the base environment, neither the Conda installed library nor our development library were available in the scipydev environment. Unsure if anaconda has changed this in the last few months, or if I'm missing something entirely. Two solutions were found, however.
The first is a quick workaround, and not feasible for long-term development. This includes using the Spyder app (in either the base or scipydev environment) to edit our PYTHONPATH. The downside is that Spyder seemingly uses this PYTHONPATH for all development environments, and will override the Conda installed SciPy library in the base environment.
The second solution is built into Anaconda, and appears to be the 'correct' way of doing this. Using the command "conda develop /path-to-scipy", Anaconda creates a .pth file within the ~/anaconda3/envs/scipydev/lib/pythonX.X/site-packages/ directory. On startup, Anaconda adds any newline-separated paths found in any .pth files to our environment path. Spyder will then use our development-SciPy library when in our scipydev environment, and the Conda installed SciPy library when in our base environment.
I would look into replacing this section on '...adding our root SciPy directory...' with the second solution.
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.
More information on the "conda develop..." command can be found here.
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.
Good find! I created the video about a year ago, maybe more. It looks like I made a typo when I (recently) typed the document based on the video.
I knew about changing PYTHONPATH from within Spyder, and you can toggle that on and off, but it's not as convenient as the solution I chose to present (once you get it set up right). Anyway, I didn't find this conda develop
solution then. I'll look into it.
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.
Can you write instructions for this and indicate which existing instructions they should replace?
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.
Definitely!
|
||
#. Install `Homebrew`_. Enter into the terminal |br| ``/usr/bin/ruby -e "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/master/install)"`` |br| or follow the installation instructions listed on the Homebrew website. Homebrew is a package manager for macOS that will help you download `gcc`, the software we will use to compile C, C++, and Fortran code included in SciPy. | ||
|
||
#. Use Homebrew to install ``gcc`` by entering the command ``brew install gcc``. |
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.
Compared to the old documentation on getting the correct compilers (because I attempted to follow those directions very closely... for scientific reasons of course..), this is far simpler, and much harder to mess up.
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.
Great to hear!
* performing an in-place build of SciPy; and | ||
* creating a virtual environment that adds this development version of SciPy to the Python path on macOS. | ||
|
||
Its companion videos `Anaconda SciPy Dev: Part I (macOS)`_ and `Anaconda SciPy Dev: Part I (macOS)`_ show many of the steps being performed. This guide may diverge slightly from the videos over time with the goal of keeping this guide the simplest, up-to-date procedure. |
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.
The companion videos are extremely helpful! Watching and pausing them as I went through each respective step helped ensure I was doing the correct things.
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.
Great.
|
||
#. (Optional) Check your present working directory by entering ``pwd`` at the terminal. You should be in the root ``/scipy`` directory, not in a directory ending ``/scipy/scipy``. | ||
|
||
#. Do an in-place build: enter ``python3 setup.py build_ext --inplace``. |br| This will compile the C, C++, and Fortran code that comes with SciPy. We installed ``python3`` with Anaconda. ``setup.py`` is a script in the root directory of SciPy, which is why you have to be in the SciPy root directory to call it. ``build_ext`` is a command defined in ``setup.py``, and ``--inplace`` is an option we'll use to ensure that the compiling happens in the SciPy directory you already have rather than some other folder on your computer. |
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.
Explaining what the command does and what the flags do is a good move, especially for those who haven't built SciPy or similar libraries before.
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.
The steps for rendering documentation locally with Sphinx are fairly straightforward, and they work well.
|
||
.. note:: | ||
|
||
Changes to certain documents do not take effect when Sphinx documentation |
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.
Do we know what kinds of changes exhibit this behavior, or is it seemingly arbitrary?
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.
I know it happens when updating documentation of solver methods, but I'm not sure if that's the only time, so I didn't speculate. If someone else has an answer it would be good to know.
@larsoner CI is happy and the toctrees are gone. I still need to make the modifications Fletcher suggested, but they are relatively small. Update: all set. |
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.
Looks like a useful refactoring, I added a few sphinx
-related suggestions that might make it a bit more streamlined (highlight
and intersphinx
use) and easier to understand for the next person to edit (rename included files as .inc
and remove them from excludes
).
doc/source/conf.py
Outdated
'dev/releasing.rst', | ||
'dev/versioning.rst', | ||
# these are all included directly in dev/core-dev/index.rst: | ||
'dev/core-dev/decisions.rst', |
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.
Maybe the thing to do would be to rename the files with .rst.inc
extension to make it clear that they are just meant to be included and not rendered/used on their own. Then they don't even need to be in this list because they won't match the doc extension (rst)
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.
After renaming the files, removing the exclude patterns, modifying the include directives, e.g.
.. include:: decisions.rst.inc
and waiting for it to rebuild from scratch, I found /dev/core-dev/index.html
to be empty. Reverting and waiting again : (
I'd prefer not to try again with this PR, if that's OK? Maybe we can make this an issue?
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.
Saw your PR so I tried again.
The problem was that when I attempted to change the filename extension, macOS appended .rst
onto it again and then automatically turned on "Hide filename extension" for that file. So my files were actually .rst.inc.rst
but it only showed me .rst.inc
, as though I had successfully changed the extension.
No ./
because all the files are in the same directory as index.rst
.
|
||
.. _NumPy/SciPy Testing Guidelines: https://docs.scipy.org/doc/numpy/reference/testing.html | ||
|
||
.. _numpydoc docstring guide: https://numpydoc.readthedocs.io/en/latest/format.html |
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.
After making sure numpydoc
is in our intersphinx
this can be
:label:`numpydoc:format`
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.
How about :ref:numpy:howto-document
?
|
||
#. Log into CircleCI with your GitHub account. | ||
#. Visit the “ci/circleci: build_docs” job page as explained above. | ||
#. Select the “Artifacts” tab and find ``index.html``. |
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.
All of this can be removed, given the above.
FYI any GitHub user can also access https://circleci.com/gh/scipy to see all builds, but they probably don't care in most cases.
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.
Maybe that's new? At some point I think that URL trick was needed. Thanks.
It is in 56 MAJOR = 1
57 MINOR = 4
58 MICRO = 0
59 ISRELEASED = False
60 VERSION = '%d.%d.%d' % (MAJOR, MINOR, MICRO) |
Thanks @tylerjreddy didn't see that before. I'm adding it to the checklist. |
Adapted existing NumPy documentation for SciPy. Changes are mostly trivial, but should be helpful to new contributors.
Added contributor-toc.rst; link to it from source/index.rst and HACKING.rst.txt. Remove "Recommended development setup", "SciPy structure", checklist, links, and FAQ from HACKING.rst.txt. They will be added to other sections as appropriate. HACKING.rst.txt is a good introduction to how to contribute, but it does not prepare the new reader for actually using all the rest of this information.
Added macOS Development Setup Quickstart guide Moved ecommended Development Setup from HACKING.rst.txt to new document
New SciPy contributors are likely to think of themselves as "Developers", but this guide is geared toward Core Developers. The name made it sound like it was a good place to start learning about developing for SciPy, but it isn't. I hope this slight change helps to distinguish it from the rest of the contributor documentation.
7b1b785
to
709d4d7
Compare
@Kai-Striega @larsoner I've responded to comments and cleaned commit history. Can we merge before the sprint at the SciPy conference (July 13/14)? |
Sounds good to me.
…On Mon., 1 Jul. 2019, 08:33 Matt Haberland, ***@***.***> wrote:
@Kai-Striega <https://github.com/Kai-Striega> @larsoner
<https://github.com/larsoner> I've responded to comments and cleaned
commit history. Can we merge before the sprint at the SciPy conference
(July 13/14)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10286?email_source=notifications&email_token=AHFOJNIFRBSBIC2NH6UDTBTP5FGEZA5CNFSM4HWPVCI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY4XAMQ#issuecomment-507080754>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHFOJNMF2X43PZM4HSJQFFTP5FGEZANCNFSM4HWPVCIQ>
.
|
I added gh-10386 with instructions on adding a new Does this Overhaul have an obvious place for such instructions? |
@pvanmulbregt adding those is a great idea. There is definitely a good place for them, but yeah - this PR is already massive, so let's get this merged and then you can fit these documents into the new table of contents. @Kai-Striega @larsoner I tried to clean the commit history after responding to your comments. Look ready to go? If so, would one of you hit the green button? |
@mdhaber just confirming merge or squash & merge? |
@Kai-Striega I've tried to keep the commits logical. If you think they look OK to you, regular merge is good. |
@mdhaber, a belated thanks for this! Earlier in the discussion you asked about the file Is there any reason to not merge these? I can do it if there is no reason not to. |
@WarrenWeckesser note that @rgommers wrote on June 10:
If it's OK to break from history and not have a |
Closes #8353
Closes #8966
Closes #9951
Closes #10355
Partially addresses #8575, but information about using Cython as a bridge to other compiled code should be added.
development_workflow.rst
and all files in the/gitwash
adapted from NumPy documentation.Most existing SciPy documentation has been preserved, but may have been moved (e.g. parts from
HACKING.rst.txt
have been redistributed). Please see self-review for more information.Questions:
scipy
root directory? Why not just put the contents in\doc\source\hacking.rst
? Just curious.SCIPY_XSLOW=1
as an OS environment variable?TODO: