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

Update Gammapy packaging, removing astropy-helpers #2279

Merged
merged 17 commits into from Aug 27, 2019
Merged

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 14, 2019

This PR updates and modernises the Gammapy packaging.

It removes astropy-helpers and Astropy package-template things, replacing them with general Python tools used by many packages. This has the advantage of not having to deal with the astropy_helpers git sub-module any more, or to periodically update that submodule and related files in the Gammapy repository to follow fixes and changes.

There is astropy/astropy-APEs#52 which is basically proposing the changes I'm applying here. In detail there's a few things we should discuss and consider. E.g. I would suggest that we keep it very simple and don't add a pyproject.toml or tox.ini for now, but just stick with our Makefile. The difference is that pyproject.toml with pip install and tox will cause virtual environments to be created in the background, that contain the build dependencies. We have the environment-dev.yaml and a fully working local development environment, I don't think we need or want those extra envs. This will need some thought when it comes to distribution of sdist and install of those on the user machine. I'll investigate this a bit and try to add CI builds here that test if pip install works -- we might need pyproject.toml in the end to have the build dependencies (setuptools_scm, cython, numpy) that we already import in setup.py. If possible I would like to avoid using the new proposed extension_helpers, I think having ~ 10 lines to configure our Cython build in setup.py is preferable. Probably we continue cythonising before release and shipping the C code for now, although this could be changed either way.

Impact for Gammapy users: No change for users with a modern setuptools / pip version. I'm not sure what happens for users with old versions of setuptools / pip - I think we can give a good error message and they have to do pip install -U setuptools pip and then try pip install gammapy again. For conda it's a non-issue, nothing changes.

Impact for Gammapy developers: almost nothing would change, except that they don't have to deal with astropy_helpers git submodule issues any more. The command to build the docs changes.

Impact for Gammapy maintainers: simpler, more standard setup. We have to sort out CI and release issues once, a one-time investment for a better future.

This will need some iteration for sure to get everything working, not ready for review / merging yet.

@cdeil cdeil added this to the 0.13 milestone Jul 14, 2019
@cdeil cdeil requested review from adonath and bsipocz Jul 14, 2019
@cdeil cdeil self-assigned this Jul 14, 2019
setup.cfg Outdated Show resolved Hide resolved
@cdeil
Copy link
Member Author

@cdeil cdeil commented Jul 17, 2019

There's one Sphinx warning concerning _static that appears after the changes in this PR:

https://dev.azure.com/gammapy/gammapy/_build/results?buildId=1286&view=logs&jobId=562d890d-bc3a-5a9d-5926-900cbb5c7a53&taskId=88fcd0ae-b4a0-5b13-41ea-01b2ecb64e72&lineStart=1223&lineEnd=1228&colStart=1&colEnd=1

Probably we have to edit docs/conf.py and change something concerning _static and the files there?

Some tips: https://blog.readthedocs.com/custom-css-and-js-in-sphinx/

We can require latest Sphinx for docs build, no need to support old versions.

@Bultako - Could you please have a look?

The other change needed here is to change the travis-ci tests to not run via python setup.py test and more, but to use pytest ... directly, and to update the devdocs. So I think finishing this up will have to wait until next week.

Copy link
Member

@bsipocz bsipocz left a comment

In general this LGTM, though I just run through the diff without trying out locally.

@Bultako
Copy link
Member

@Bultako Bultako commented Jul 18, 2019

@cdeil
There are many others warnings appearing related with missing notebooks i.e. "WARNING: Unknown notebook spectrum_analysis" These warnings, as well as the one you mention, disappear when you run your build docs command from within the docs folder.

$ cd docs
$ time python -m sphinx . _build/html -b html
real	3m29.066s
user	2m57.359s
sys	0m17.702s

I do not neither have the warnings building the docs when running either of the following commands from the root base:

$ time python setup.py build_docs
real	3m21.943s
user	2m53.952s
sys	0m17.526s
$ time make docs-all nbs=False
real	3m40.555s
user	3m4.154s
sys	0m21.291s

@cdeil
Copy link
Member Author

@cdeil cdeil commented Jul 18, 2019

So we should cd docs before running sphinx-build?
I think there is a way to do this in the Makefile, only for that one command.

When working on this I looked at this example, and I think it's not doing cd docs, it's executing sphinx-build from the repo level. That's why I'm confused as to what to do.
https://github.com/aperiosoftware/aas-timeseries/pull/29/files#diff-b91f3d5bd63fcd17221b267e851608e8R13

@cdeil cdeil removed this from the 0.13 milestone Jul 18, 2019
@cdeil cdeil added this to the 0.14 milestone Jul 18, 2019
@Bultako
Copy link
Member

@Bultako Bultako commented Jul 18, 2019

I've spent quite some time trying to understand why Sphinx says at the end that it does not find the _static folder and at the same time builds the pretty docs using all that is provided in the _static folder. I still do not get it :(

The only solution I got now is to change the Makefile as below.
I've tested and it works fine.

docs-sphinx:
	cd docs && python -m sphinx . _build/html -b html

@cdeil
Copy link
Member Author

@cdeil cdeil commented Aug 26, 2019

@Bultako - Thank you for the investigation and tip for the Sphinx build. Applied your suggested fix in 47e3ae9 .

@cdeil cdeil assigned adonath and unassigned cdeil Aug 26, 2019
@cdeil
Copy link
Member Author

@cdeil cdeil commented Aug 26, 2019

@adonath @bsipocz - This is ready for review.

Some possible improvements:

  • Update dev docs, especially this page
  • Add pyproject.toml? (see astropy/astropy-APEs#52 (comment))
  • Change (some, all) tests to run from an installed version instead of in-place?
  • Add build using our dev-environment.yml

Let me know what you think about those, or any other suggestions you have.

Note that in a few weeks once #2218 is agreed, we'd have to update and adjust CI anyways, to test against the oldest supported versions explicitly. So I don't want to spend too much time on CI improvements now, but of course, we should improve a bit, especially if there's something important.

@cdeil
Copy link
Member Author

@cdeil cdeil commented Aug 27, 2019

CI is green now. Would prefer to merge this in now and defer work on dev docs to later (and discuss a bit about dev docs structure & content and then do a rewrite).

Copy link
Member

@adonath adonath left a comment

So far I don't have any comments. I'm fine to merge now and try this out in master (rebasing my old branches...). Or do you want me to check this out on the side now?

@cdeil cdeil merged commit e3524c5 into gammapy:master Aug 27, 2019
9 checks passed
@cdeil
Copy link
Member Author

@cdeil cdeil commented Aug 27, 2019

Follow-up commit: 1eb9a84

@cdeil
Copy link
Member Author

@cdeil cdeil commented Aug 27, 2019

So for me, looks like this worked OK:

git checkout master
git pull
rm -r astropy_helpers

https://gist.github.com/cdeil/47716c65ac6e3d2aa4370467dc3d57e0

I did not try switching back and forth yet to open feature branches that still have astropy-helpers.

@adonath - Maybe you can also try out the transition a bit, and once we have some simple instructions for developers we post a suggestion in Slack.

@adonath
Copy link
Member

@adonath adonath commented Aug 27, 2019

@cdeil The rebase also worked for me:

git fetch origin
git rebase origin/master
rm -r astropy_helpers

Switching back and forth from old feature branches just works. The astropy_helpers sub-folder is tracked correctly. So the recommendation could just be to rebase open feature branches (which you have to do anyway...), remove astropy_helpers and that's it?

@cdeil
Copy link
Member Author

@cdeil cdeil commented Aug 27, 2019

As far as I can see this is the only PR that needs a rebase: #2323 (comment)

@cdeil
Copy link
Member Author

@cdeil cdeil commented Aug 27, 2019

Another follow-up commit: e5a2a1a

Removes gammapy/utils/setup_package.py.
Package data files are now declared in setup.cfg here:

gammapy/setup.cfg

Lines 67 to 70 in e5a2a1a

[options.package_data]
gammapy.irf.tests = data/*
gammapy.utils.serialization.tests = data/*
gammapy.tests = coveragerc

I think the coverage config might not be reference correctly ... checking now.

@cdeil
Copy link
Member Author

@cdeil cdeil commented Aug 27, 2019

Actually, coverage config seems OK, it was still being picked up.

Since pytest config and most other config is now centralised in setup.cfg, I've also moved the coverage config there, getting rid of the extra gammapy/test/coveragerc in 52727ce

Let's see if this works OK on travis-ci and coveralls ...

@cdeil
Copy link
Member Author

@cdeil cdeil commented Aug 27, 2019

Looks like 52727ce worked, zero changes to coverage results: https://coveralls.io/builds/25365981

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants