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

Add Readthedoc feature #15

Merged
merged 62 commits into from
Jun 17, 2024
Merged

Add Readthedoc feature #15

merged 62 commits into from
Jun 17, 2024

Conversation

shenvitor
Copy link
Member

@shenvitor shenvitor commented Jun 4, 2024

Make it now have both GitHub documentation and RTD build

@shenvitor shenvitor self-assigned this Jun 4, 2024
@shenvitor shenvitor requested a review from redeboer June 4, 2024 16:22
@shenvitor
Copy link
Member Author

shenvitor commented Jun 4, 2024

I want to add this...
and I have authorised in : https://readthedocs.org/projects/gluex-nstar/

seems like it is necessary to add the .readthedocs.yaml file, and that is enough(?) @redeboer

@shenvitor shenvitor added the 🖱️ DX Improvements to the Developer Experience label Jun 4, 2024
@redeboer
Copy link
Member

redeboer commented Jun 4, 2024

Fancy! Not sure if RTD is really needed. Usually it is used for packages that need versioning (so a page per version, e.g. https://qrules.rtfd.io/0.10.1). Another benefit is PR previews. If you don't need those, GitHub Pages suffices. (GH has the benefit that it can cache e.g. notebook results and sympy caches.)

@redeboer
Copy link
Member

redeboer commented Jun 4, 2024

But let's discuss tomorrow, there is something to say for using RTD ✨

@shenvitor
Copy link
Member Author

Fancy! Not sure if RTD is really needed. Usually it is used for packages that need versioning (so a page per version, e.g. https://qrules.rtfd.io/0.10.1). Another benefit is PR previews. If you don't need those, GitHub Pages suffices. (GH has the benefit that it can cache e.g. notebook results and sympy caches.)

Yup actually I wanna have PR preview, that's why I wanna add it :)

@shenvitor
Copy link
Member Author

shenvitor commented Jun 6, 2024

OK, now I understand. Maybe "rtd" is not necessarily appropriate here, but I'll decide later and make this work first. Seems like it is Okish

@shenvitor shenvitor marked this pull request as draft June 6, 2024 13:52
@redeboer
Copy link
Member

redeboer commented Jun 6, 2024

Have a look at the log output. The config is looking for docs/source/conf.py, not docs/conf.py
https://readthedocs.org/projects/gluex-nstar/builds/24613998/

@shenvitor
Copy link
Member Author

shenvitor commented Jun 6, 2024

Interestingly... I see at some point the readthedoc is okay but the Github pages failed :(
https://readthedocs.org/projects/gluex-nstar/builds/24613277/
Not sure if I can have both(?)
extension of mystnb and sphinx of readthedoc

@shenvitor
Copy link
Member Author

shenvitor commented Jun 7, 2024

Man.... Finally now in this version both the GitHub documentation and RTD are working :)
(waiting for approval @redeboer 🐳)

@shenvitor shenvitor marked this pull request as ready for review June 7, 2024 09:34
Copy link
Member

@redeboer redeboer left a comment

Choose a reason for hiding this comment

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

Good job!
There are some files that seem redundant to me, have a look at the comments ;)

docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@
linkify-it-py
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this duplication of dependencies. Indeed RTD by default expects dependencies to be defined through pip requiremente files, but you can define custom build steps. In this case to specify you want to run pixi shell before the build process. Have a look at e.g. https://github.com/ComPWA/ampform for how a custom build (in that case with pip constraint files) looks like.

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 see... right now kinda stuck in here 🐥

.github/workflows/ci-docs.yml Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/make.bat Outdated Show resolved Hide resolved
pyproject.toml Outdated
@@ -0,0 +1,8 @@
[build-system]
Copy link
Member

Choose a reason for hiding this comment

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

Was this required by rtd? If so, I would convert pixi.toml to pyproject.toml
https://pixi.sh/latest/advanced/pyproject_toml

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 removed and it works.

post_install:
- curl -fsSL https://pixi.sh/install.sh | bash
- export PATH="$HOME/.pixi/bin:$PATH" && pixi install
- pixi shell -c "pixi install && sphinx-build -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html"
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, RTD runs its own sphinx-build command (hence pre_build, post_build, not build itself). So just running pixi shell should suffice 🤞 If it doesn't, this may be a more tricky issue to solve...

Copy link
Member

Choose a reason for hiding this comment

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

Btw pixi install is not needed, at most pixi shell

Copy link
Member Author

@shenvitor shenvitor Jun 12, 2024

Choose a reason for hiding this comment

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

Unfortunately, RTD runs its own sphinx-build command (hence pre_build, post_build, not build itself). So just running pixi shell should suffice 🤞 If it doesn't, this may be a more tricky issue to solve...

Seems like it doesn't, maybe tricky? What if the pixi shell an interactive shell which is not supported in the ReadTheDocs environment, maybe?

e.g. this issue in RTD build indicates that it probably can't start an interactive shell there:
Screenshot 2024-06-12 at 11 21 22

Copy link
Member Author

Choose a reason for hiding this comment

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

build-docs.sh Outdated
@@ -0,0 +1,3 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

This file can be removed, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup originally just testing

docs/conf.py Outdated Show resolved Hide resolved
pixi.toml Outdated
myst-nb = ">=1.1.0,<1.2"
myst-parser = ">=3.0.1,<3.1"
nbsphinx = ">=0.9.4,<0.10"
Copy link
Member

Choose a reason for hiding this comment

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

I would use myst-nb instead of nbsphinx (that's a different notebook renderer). That also removes the need for myst-parser, maybe also that linkify-md thing, because those are indirect dependencies of myst-nb

Copy link
Member

Choose a reason for hiding this comment

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

Actually, why are dependency changes required in this PR?

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 added them when I added RTD from their example

pixi.toml Outdated
sphinx-autobuild = ">=2024.4.16,<2024.5"
sphinx_rtd_theme = ">=2.0.0,<2.1"
Copy link
Member

Choose a reason for hiding this comment

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

Are you selecting that in conf.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I have

docs/index.md Outdated

Investigation of the N\* Resonances in the GlueX Experiment

> **Note:** This project is under active development.
Copy link
Member

Choose a reason for hiding this comment

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

I would use a MyST admonition here

docs/conf.py Outdated
"sphinx.ext.napoleon",
"sphinx.ext.viewcode",
]
html_theme = "sphinx_rtd_theme"
Copy link
Member

Choose a reason for hiding this comment

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

This theme change is not required on RTD. Also works with what you had before.

@shenvitor shenvitor changed the title Add Readthedoc Attempt: add Readthedoc Jun 12, 2024
@redeboer
Copy link
Member

Hmm seems that this is a bit more complicated then expected. I think the best way forward is to convert pixi.toml to pyproject.toml, so that the dependencies can be installed through pip (or uv) directly (which RTD requires/prefers). But let's look at that once I'm back

@redeboer
Copy link
Member

redeboer commented Jun 14, 2024

@shenvitor check this newpixi release. There are some examples now of how to use pixi in Read the Docs:
https://github.com/prefix-dev/pixi/blob/main/examples%2Freadthedocs-override%2F.readthedocs.yaml

@shenvitor

This comment has been minimized.

@redeboer
Copy link
Member

redeboer commented Jun 17, 2024

I think you can replace the entire .readthedocs.yml with this part:
https://github.com/prefix-dev/pixi/blob/f8f84d305673976600689aca74d8bdc312b63561/examples/readthedocs-override/.readthedocs.yaml#L1-L10
Of course, do replace the docs task name with docnb.

Forget the last line with pixi run readthedocs for now. Maybe we need some modification of that task later on, but let's see if it works without.

@shenvitor shenvitor changed the title Attempt: add Readthedoc Add Readthedoc feature Jun 17, 2024
Copy link
Member

@redeboer redeboer left a comment

Choose a reason for hiding this comment

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

Awesome

@shenvitor shenvitor marked this pull request as ready for review June 17, 2024 13:52
@shenvitor shenvitor merged commit 5b088f2 into main Jun 17, 2024
4 checks passed
@shenvitor shenvitor deleted the readthedoc branch June 17, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🖱️ DX Improvements to the Developer Experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants