astropy / halotools

Python package for studying large scale structure, cosmology, and galaxy evolution using N-body simulations and halo models
http://halotools.rtfd.org
BSD 3-Clause "New" or "Revised" License
104 stars 65 forks source link

Implement pyproject.toml #1041

Closed aphearin closed 2 years ago

aphearin commented 2 years ago

This WIP PR attempts to resolve RTD build failures.

aphearin commented 2 years ago

@pllim Could you have a look at docs/conf.py and .readthedocs.yml to see if you understand why I would be getting this error when building the halotools docs on RTD ModuleNotFoundError: No module named 'numpy'

pllim commented 2 years ago

@aphearin , could you please enable RTD PR build so we can debug this here?

https://docs.readthedocs.io/en/stable/pull-requests.html

On the GitHub side, under Settings, go to RTD webhook and make sure it is sending PR event. Then you can add/amend commit here and see if it pops up under the CI list. Thanks!

aphearin commented 2 years ago

Hmm, I enabled the webhook (checking Push to Repository and also Pull Request), and then I pushed up a forgotten change to tox.ini, but there doesn't seem to be a CI check that is showing up. However, on RTD, when I inspect the webhook, I see that this push shows up under Recent Activity.

aphearin commented 2 years ago

Well, hang on, forgot to hit save on the "build pull requests" option on the RTD admin page. Sorry, one sec...

aphearin commented 2 years ago

Ok, that did the trick - RTD build is now showing up as a CI check.

pllim commented 2 years ago
      error: [Errno 2] Could not find C/C++ file
halotools/mock_observables/tensor_calculations/engines/inertia_tensor_3d_engine.(c/cpp) for Cython file
halotools/mock_observables/tensor_calculations/engines/inertia_tensor_3d_engine.pyx when building extension
halotools.mock_observables.tensor_calculations.engines.inertia_tensor_3d_engine.
Cython must be installed to build from a git checkout.:
'halotools/mock_observables/tensor_calculations/engines/inertia_tensor_3d_engine.c'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: legacy-install-failure
pllim commented 2 years ago

Looks like you are not using pyproject.toml yet. I wonder if your current setup is too old for RTD to understand. It did not seem to pick up Cython requirement from your requirements.txt . I would say look at how astroquery is doing it (https://github.com/astropy/astroquery/blob/main/.readthedocs.yaml) since it is also still using astropy-helpers and has a working RTD build.

aphearin commented 2 years ago

Ok, I can try and mirror what astroquery is doing. But is the migration to pyproject.toml that laborious? Do you think it would be better to just go ahead and handle that migration before releasing? Or will that require overhauling the helpers dependencies in a way that will be very time-consuming?

aphearin commented 2 years ago

Actually, astroquery is using pyproject.toml - thanks for the pointer I'll see if I can just mirror what they're doing and see how it goes

pllim commented 2 years ago

But is the migration to pyproject.toml that laborious?

I would say a couple of hours but it depends on your familiarity with it.

Do you think it would be better to just go ahead and handle that migration before releasing?

Probably. If RTD cannot build it, chances are you will have problem building wheel for it as well during release. Or your users will have problem installing it if their pip is too new or something (just wildly guessing here).

Or will that require overhauling the helpers dependencies in a way that will be very time-consuming?

Probably not. Would be nice but it would require maybe a couple of days on-and-off working on it.

aphearin commented 2 years ago

@pllim I've taken a stab at migrating to pyproject.toml and I've tried to follow the astroquery example. Both my CI Tests and RTD workflows have the same error:

ImportError: cannot import name 'setup' from 'astropy_helpers.setup_helpers' (/home/docs/checkouts/readthedocs.org/user_builds/halotools/checkouts/1041/astropy_helpers/astropy_helpers/setup_helpers.py)

My setup.py file is the same as the one in astroquery. I think this is the line that causes the issue with all 3 workflows. I thought this might be due to an out-of-date version of the helpers, although I can see in the RTD traceback that there is a submodule update invocation before this error appears. Any ideas for how to proceed?

pllim commented 2 years ago

Looks like astroquery has helpers at https://github.com/astropy/astropy-helpers/tree/d2a6304a3e801bc2cb053ccf1cf09a9f1e62036c while yours is at https://github.com/astropy/astropy-helpers/tree/fd80be7cf8698e2350a919ef7fc89871db0ba580 . You might need to update your submodule?

aphearin commented 2 years ago

Yes, thanks, updating to the same version of the helpers used by astroquery resolved the CI Tests (man, my helpers had gotten really out of date). So good news is halotools has now successfully migrated to pyproject.toml! That was really worth doing - this migration will be easy for my other packages now that I've done it for a pretty nontrivial example.

Bad news is that switching to pyproject.toml did not resolve the RTD fail. I still get the same error message: ModuleNotFoundError: No module named 'numpy'

pllim commented 2 years ago

So... I just realized that astroquery does not have to worry about C code like you do. Perhaps it is not quite possible to modernize your build infrastructure without applying APE 17. We are not longer pushing updates to astropy-helpers and looks like the build error still bubbles up from there even after you update the helpers to version used by astroquery. 😿

pllim commented 2 years ago

Oh, wait wait... maybe try this and see:

https://github.com/astropy/astropy/blob/3912916dad56920514ba648be400a5f82add041a/pyproject.toml#L2-L7

aphearin commented 2 years ago

I think the build requirements issue is now resolved thanks to the suggestion of @pllim to look at these lines of the astropy pyproject.toml file (and also the suggestion to fix the error in my Admin page of RTD, d'oh!).

I've made a few more successive failures, each time resolving the failure by making straightforward changes to my conf.py by referring to the relevant line(s) of the conf.py module in astroquery.

pllim commented 2 years ago

Looks like you need to not use astropy-helpers in docs/conf.py. Instead use https://github.com/astropy/sphinx-astropy

aphearin commented 2 years ago

Now that I am using sphinx_astropy within docs/conf.py, I have now reached the final line of conf.py, so with fingers crossed, this is the final bug to resolve:

Exception occurred:
  File "/home/docs/checkouts/readthedocs.org/user_builds/halotools/envs/1041/lib/python3.9/site-packages/halotools/utils/autosummary_workaround.py", line 6, in <module>
    class FixedAutosummary(Autosummary):
NameError: name 'Autosummary' is not defined

This bug can be traced to this line in the halotools source code, which was written by Erik way back when for reasons I forget. The FixedAutosummary class inherits from Autosummary, which appears to have been previously located somewhere in the sphinx.ext.autosummary namespace, but I guess not in more recent versions of sphinx?

I have grepped for Autosummary within the source code of the sphinx-astropy package, but I do not see where this is defined.

pllim commented 2 years ago

I would just ditch FixedAutosummary altogether and see if the doc rendering is still acceptable. All the Astropy projects use the vanilla autosummary and no one complained (that much).

pllim commented 2 years ago

Oh yeah, I think you definitely don't need it. Sphinx is like v6 now.

https://github.com/astropy/halotools/blob/3449072811aa1044ef65490f7ae8e975c89dd312/halotools/utils/autosummary_workaround.py#L111

aphearin commented 2 years ago

Whoa, yeah, good catch. Fingers crossed this does the trick.

aphearin commented 2 years ago

Well I think that ditching the autosummary_workaround is all that needs to be done on the packaging side. Unfortunately, I now have a ton of new errors showing up, presumably because some choices I made a while back pertained to grossly out-of-date version of sphinx, and these show up now that I've updated all the dependencies. So I think I still have a lot of work to do, but at least now the problems don't seem to have anything to do with packaging, only my docstring formats. I'll try and tackle the reformatting next week - thanks so much for all the help @pllim!

aphearin commented 2 years ago

It turns out that the docstring formatting issues weren't bad at all - only a single failure from a missing colon, and the rest were just warnings with simple fixes. Reiterating many thanks to @pllim for all the patience and support!