[hail][docs] fix a bunch of issues #9403
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The remainder: |
|
My current understanding is that we aren't going to merge this as is because we don't want a bunch of links to change? |
|
Most of the changes here are totally fine, but yes, I think we should hold off on changing paths. I have a to-do item to look into configuring / monkey patching the Sphinx function that's trying to generate links for the signatures. Dan is also OOO this week so not high prio. |
|
Dan, do you want to split out any parts of this that don't change links? |
|
yeah I gotta work on my presentation for ATGU and hail team meetings, but I'll try to do this later next week |
|
OK, root cause is that newer versions of autodoc include, as an experimental addition, sphinx-autodoc-typehints. This addition (which we use in batch) only works when a class is documented using its true name (i.e. where it is defined, not re-exported). A quick fix is to disable this functionality:
The autodoc-typehints maintainer seems to have gotten stuck when trying to fix this. It appears that someone went and figured out enough of Sphinx to fix this. When a doc string is processed, they record a mapping from the documented name to the fully-resolved name. The code that catches missing references and fixes them is kinda big and complicated. I'm uncomfortable dropping it into our project. There's some good documentation about how autodoc_typehints works at scanpydocs' docs. This flying sheep seems pretty competent. I think they fixed it for scanpydocs here but it's a rather complex looking solution. It's frankly pretty shocking that such a simple operation (have a mapping from all the names of an object to its documented name) results in a two years of back and forth that still hasn't reached a solution. |
We are down to 164 docs failures and I am tired. I will work on it more anohter time. Then we will be able to enable nitpicky and our docs will never have broken links/ .
|
OK. It builds, nitpicky is on. I never want to look at sphinx again. |
danking
commented
Oct 5, 2020
| @@ -4,7 +4,6 @@ | |||
|
|
|||
| .. autoclass:: {{ objname }}() | |||
| :members: | |||
| :show-inheritance: | |||
There was a problem hiding this comment.
This is a useful feature, but because our docs document things in a place other than where they are defined (e.g. CollectionExpression's docs are at hail.expr.CollectionExpression instead of hail.expr.expressions.typed_expressions.CollectionExpression`) autodoc simply does not work (it cannot resolve the reference).
danking
commented
Oct 5, 2020
| @@ -9,7 +9,7 @@ BUILDDIR = _build | |||
| # Internal variables. | |||
| PAPEROPT_a4 = -D latex_paper_size=a4 | |||
| PAPEROPT_letter = -D latex_paper_size=letter | |||
| ALLSPHINXOPTS = -W -d $(BUILDDIR)/doctrees $(PAPEROPT_$(PAPER)) $(SPHINXOPTS) . | |||
| ALLSPHINXOPTS = -d $(BUILDDIR)/doctrees $(PAPEROPT_$(PAPER)) $(SPHINXOPTS) -W --keep-going . | |||
There was a problem hiding this comment.
keep going means show me all the errors not just the first one
|
Please give this a thorough read through to ensure I didn't accidentally break anything or delete anything. I'd also appreciate if you spot-checked the docs a few times (you'll have to build them locally). |
|
thanks so much for doing this Dan! |
|
Seconding the thank you. I'll build locally and go through everything today. |
johnc1231
suggested changes
Oct 6, 2020
There was a problem hiding this comment.
One small thing I hit right away: Let's have hail-docs-no-test depend on install-dev-deps. Otherwise nothing makes sure you have proper sphinx installed.
Once I did that, I tried to build and get a wall of:
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/expr/aggregators/aggregators.py:docstring of hail.expr.aggregators.filter:18: WARNING: py:class reference target not found: Aggregable
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/expr/aggregators/aggregators.py:docstring of hail.expr.aggregators.call_stats:46: WARNING: py:class reference target not found: ArrayStringExpression
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/expr/aggregators/aggregators.py:docstring of hail.expr.aggregators.downsample:8: WARNING: py:data reference target not found: ttuple
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/expr/aggregators/aggregators.py:docstring of hail.expr.aggregators.downsample:8: WARNING: py:data reference target not found: tarray
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/expr/aggregators/aggregators.py:docstring of hail.expr.aggregators.downsample:8: WARNING: py:data reference target not found: tstring
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/context.py:docstring of hail.init:54: WARNING: py:obj reference target not found: dict[str, str]
/Users/johnc/Code/hail/hail/python/hail/docs/experimental/index.rst:53:<autosummary>:1: WARNING: py:class reference target not found: Expression
/Users/johnc/Code/hail/hail/python/hail/docs/experimental/index.rst:53:<autosummary>:1: WARNING: py:meth reference target not found: experimental.write_expression
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/ld_score_regression.py:docstring of hail.experimental.ld_score_regression:69: WARNING: py:class reference target not found: Table
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/ld_score_regression.py:docstring of hail.experimental.ld_score_regression:69: WARNING: py:class reference target not found: MatrixTable
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/ld_score_regression.py:docstring of hail.experimental.ld_score_regression:75: WARNING: py:data reference target not found: tarray
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/ld_score_regression.py:docstring of hail.experimental.ld_score_regression:96: WARNING: py:data reference target not found: tarray
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/ld_score_regression.py:docstring of hail.experimental.ld_score_regression:113: WARNING: py:class reference target not found: Table
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/expressions.py:docstring of hail.experimental.write_expression:15: WARNING: py:class reference target not found: Expression
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/expressions.py:docstring of hail.experimental.read_expression:1: WARNING: py:class reference target not found: Expression
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/expressions.py:docstring of hail.experimental.read_expression:1: WARNING: py:meth reference target not found: experimental.write_expression
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/expressions.py:docstring of hail.experimental.read_expression:11: WARNING: py:class reference target not found: Expression
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/plots.py:docstring of hail.experimental.hail_metadata:5: WARNING: py:class reference target not found: bokeh.models.widgets.panels.Tabs
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/plots.py:docstring of hail.experimental.plot_roc_curve:15: WARNING: py:class reference target not found: Figure
/usr/local/anaconda3/envs/hail/lib/python3.7/site-packages/hail/experimental/phase_by_transmission.py:docstring of hail.experimental.phase_by_transmission:25: WARNING: py:meth reference target not found: experimental.phase_trio_matrix_by_transmission
......
So maybe that's still not the proper sphinx?
johnc1231
previously requested changes
Oct 6, 2020
| @@ -385,7 +385,7 @@ def default_reference(): | |||
| return Env.hc().default_reference | |||
|
|
|||
|
|
|||
| def get_reference(name) -> 'hail.ReferenceGenome': | |||
There was a problem hiding this comment.
Why'd we get rid of this?
Comment on lines
+1943
to
+1958
| def count(self, value): | ||
| """Do not use this method. | ||
|
|
||
| This only exists for compatibility with the Python Sequence abstract | ||
| base class. | ||
| """ | ||
| return super().count() | ||
|
|
||
| def index(self, value, start=0, stop=None): | ||
| """Do not use this method. | ||
|
|
||
| This only exists for compatibility with the Python Sequence abstract | ||
| base class. | ||
| """ | ||
| return super().index() | ||
|
|
There was a problem hiding this comment.
Maybe we should throw exceptions here? Are you adding these for any functional reason, or just to appease sphinx?
There was a problem hiding this comment.
I don't understand why, but the class2.rst template somehow trigger Sphinx to generate documentation based on the inherited docstrings. This documentation comes from Python core. That documentation does not conform to rST. That documentation fails the build.
IMHO, we shouldn't inherit from Sequence if we don't want these methods. Compilers team should decide if Expressions should inherit things like Sequence.
Comment on lines
+3202
to
+3207
| alleles: :class:`.Int32Expression` or :class:`.ArrayExpression` of :obj:`.tstr`. | ||
| Number of total alleles, including the reference, or array of variant alleles. | ||
|
|
||
| Returns | ||
| ------- | ||
| :class:`.ArrayInt32Expression` | ||
| :class:`.ArrayExpression` of :obj:`.tint32` |
There was a problem hiding this comment.
I almost wonder if we should say these are arrays of StringExpression and or Int32Expression. That's the thing someone would likely want to be linked to if they were doing something with an element of this array.
There was a problem hiding this comment.
Either is fine with me, but ArrayInt32Expression ain't a thing.
| information. | ||
| Phenotype information is not preserved in the Pedigree data | ||
| structure in Hail. Reading and writing a PLINK .fam file will | ||
| result in loss of this information. Use the key table method |
There was a problem hiding this comment.
You still say "key table", though you've taken out the dead KeyTable link.
|
install-dev-deps doubles the "no change" build time from 4s to 8s on my machine. I already thing 4s is too long. I'd rather not add that. |
This reverts commit ac2174e.
danking
dismissed
johnc1231’s stale review
builds, didn't address the issue of how to document types of expressions nor what to do about inherited methods
johnc1231
approved these changes
Oct 7, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We are down to 164 docs failures and I am tired. I will work on it more another time. Then we will be able to enable nitpicky and our docs will never have broken links.