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

have readthedocs install with , drop requirements files #255

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

newville
Copy link
Member

@newville newville commented Jul 14, 2024

  • Closes readthedocs docs not building #254
  • Executed pre-commit run --all-files with no errors
  • The change is fully covered by automated unit tests
  • Documented in docs/ as appropriate
  • Added an entry to the CHANGES file

This changes the readthedocs config to install uncertainties and dependencies with pip install ".[all]".

Copy link

codecov bot commented Jul 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.33%. Comparing base (4a470e4) to head (cda9a62).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #255   +/-   ##
=======================================
  Coverage   96.33%   96.33%           
=======================================
  Files          15       15           
  Lines        1909     1909           
=======================================
  Hits         1839     1839           
  Misses         70       70           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jagerber48
Copy link
Contributor

This is my first time checking out readthedocs config. On the readthedocs page for lmfit/uncertainties I see the builds are failing. And I don't see the build for this branch. I can't see the settings for readthedocs. Is there a different readthedocs page somewhere? I see lebigot as a maintainer but no one else. Could others be added as maintainers? @newville, you've done a lot of docs work, have you been able to access readthedocs settings?

@newville
Copy link
Member Author

@jagerber48

This is my first time checking out readthedocs config. On the readthedocs page for lmfit/uncertainties I see the builds are failing. And I don't see the build for this branch. I can't see the settings for readthedocs.
Is there a different readthedocs page somewhere?

Not that I am aware of.

I see lebigot as a maintainer but no one else. Could others be added as maintainers? @newville, you've done a lot of docs work, have you been able to access readthedocs settings?

No, I do not.

@jagerber48
Copy link
Contributor

Ok got it. There's uncertainties.readthedocs and legacy uncertainties-python-package.readthedocs. I'll ignore the latter, though it does show up first on google etc.

Regarding readthedocs installation issues.

  • I worry that this will cause readthedocs to try to pip install uncertainties from pypi rather than pip installing the local version.
  • I worry that readthedocs will lack the sphinx packages it needs which are currently found in the requirements.txt file that it uses

My recommended fix is to add . to the requirements.txt file so that the readthedocs build runs pip install . in its local uncertainties directory. When it does this uncertainties will get built, including the package info file which is needed for reading the version using importlib.metadata.version in conf.py. I have this setup at #257

I don't know if this is the "best" strategy but it works for sciform.

@jagerber48
Copy link
Contributor

Ah, I see the doc requirements are included in .[all]. This fix is good then.

@@ -29,4 +29,4 @@ sphinx:
# See https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html
python:
install:
- requirements: doc/requirements.txt
- pip install ".[all]"
Copy link
Contributor

Choose a reason for hiding this comment

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

How about pip install ".[doc]" or python -m pip install ".[doc]"? No need to spend build resources installing numpy when sphinx should only need the doc building requirements.

Also this line could be indented two spaces I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

a) let's allow the assumption that numpy and pytest are installed when building the docs, in case we or anyone else wants to eventually dynamically create content (say, a gallery or jupyter directives).

b) It was not indented before, and I am not 100% sure what rtd expects.

c) It is not that I like rtd even a tiny amount, just trying to get the d*n thing to not fail. I am very much in favor of dropping rtd for GitHub pages.

@jagerber48
Copy link
Contributor

I’d like to see this run on rtd before merging but I think we would need admin privileges. Better just get this in and see if it works. @newville I’ve missed the etiquette we’ve been following for PRs lately. When would be appropriate for me or someone to merge this in?

@jagerber48 jagerber48 merged commit f097015 into master Jul 26, 2024
21 checks passed
@jagerber48 jagerber48 deleted the more_readthedocs_conf branch July 26, 2024 05:19
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.

readthedocs docs not building
2 participants