search

astropy / package-template

Template for packages that use Astropy. Maintainer: @astrofrog
http://docs.astropy.org/projects/package-template/en/latest/
Other
60 stars 63 forks source link

Refactor package template to follow APE 17 #438

Closed astrofrog closed 4 years ago

astrofrog commented 4 years ago

:construction_worker_man: This refactors the package template to follow the recommendations in APE 17. Specifically, the changes are:

In addition I have made the following changes:

:book: To see a preview of the migration guide, see https://external-builds.readthedocs.io/html/astropy-package-template/438/index.html (or click 'Details' in the status checks for the RTD build)

:warning: If you want to try out the guide, in Step 0, use the following to use the template from this PR:

cookiecutter gh:astrofrog/package-template --checkout infrastructure-refactor -o my_package_tmp

There is still some cleanup I'd like to make to the template unrelated to APE 17, so I'll leave this to another PR. In particular, @Cadair has been doing some work creating a more generic non-astropy-specific template and as part of that has developed a nice way to test the template, but that can wait for another PR.

Preview of the rendered template: https://github.com/astrofrog/package-template/tree/preview-ape17

astrofrog commented 4 years ago

(not actually ready for review, but just want to make sure the CI works correctly)

astrofrog commented 4 years ago

This is now ready for review!

astrofrog commented 4 years ago

Ok I've now simplified the migration guide a bit to avoid repeating whole files when the file can just be copied over as-is from the generated template without any modifications.

pllim commented 4 years ago

The RTD error is due to cache being incompatible with pip 20.0 upgrade. I don't know how to clear the cache in a PR auto builder. Maybe you either ignore it or force a rerun until cache clears itself? Either push commit or close/re-open to force a RTD rerun.

mwcraig commented 4 years ago

I think the problem with pip 20.0.0 is bigger than cache: https://github.com/pypa/pip/issues/7620

mhvk commented 4 years ago

@astrofrog - is the rendered version up-to-date? I ask in part since it mentions gitpython which you seem to be removing above (though perhaps I misread that comment!).

lglattly commented 4 years ago

I am not sure if @lglattly's work includes copyediting outside the core library. If yes, then maybe she could have a look at ape17.rst too?

Yes I can work on copy editing outside of the core library! @adrn can help get me set up with other repositories as needed. I can take a look at ape17.rst, but maybe it makes sense for me to do an editing pass once it's merged and open a copy editing PR for it? The addition looks long and it might be too annoying to deal with lots of small copy editing suggestions here. Let me know!

bsipocz commented 4 years ago

maybe it makes sense for me to do an editing pass once it's merged and open a copy editing PR for it

I think that's a great idea. As I see the plan is to get this merged, but don't over advertize it until the coordinated libraries are transitioned, and frankly that will take time anyway.

astrofrog commented 4 years ago

@mhvk - no the rendered version on master only gets updated when PRs like this are merged. But I've pushed a rendered version to a branch in my fork so you can see it if you like: https://github.com/astrofrog/package-template/tree/preview-ape17

astrofrog commented 4 years ago

I think I've addressed most comments - I've resolved ones that are clear cut and left others marked as unresolved.

@pllim, you said:

No mention of tagging dev for branch releases?

I don't think that belongs here - release instructions for packages (including packages using the template) are in the astropy core docs for now. I'd like to consolidate that more but for now this isn't the right place I think.

astrofrog commented 4 years ago

@pllim - thanks! I've implemented both comments.

mhvk commented 4 years ago

@astrofrog - am doing it for my baseband package now and have some comments - mostly that the cookiecutter results can be highlighted more - it does almost everything!

mhvk commented 4 years ago

p.s. Full review later today.

bsipocz commented 4 years ago

@astrofrog - I won't have time to do the migration with a package right now, so I don't expect to have any more comments than the ones you already addressed

astrofrog commented 4 years ago

@bsipocz - ok sounds good!

@mhvk - great, looking forward to it! :)

pllim commented 4 years ago

@mhvk , I prefer to have setup.cfg explanations even when cookiecutter did the work, as not everyone would like to re-run the template and move things around, especially if the existing package has other customization involved.

tepickering commented 4 years ago

i'm still in the process of my migrating my repos, but the changes here have been a HUGE help! thanks! the one issue i had was with package data being defined in both setup.cfg and MANIFEST.in. my understanding of best practices is that include_package_data = True should be set in [options] in setup.cfg and then MANIFEST.in used to configured everything that should be included. indeed, this is what i need to do to reliably package my nested directories of package and test data. is there a counter-argument for using [options.package_data] instead?

mhvk commented 4 years ago

@pllim - very much agreed! So, really all what I suggested is to mention that one can glean a lot of the needed changes from the files generated in step 0.

mhvk commented 4 years ago

With tox -e build_docs, I get the complaint:

Configuration error:
There is a programmable error in your configuration file:

Traceback (most recent call last):
  File "/data/mhvk/packages/baseband/.tox/build_docs/lib/python3.7/site-packages/sphinx/config.py", line 368, in eval_config_file
    execfile_(filename, namespace)
  File "/data/mhvk/packages/baseband/.tox/build_docs/lib/python3.7/site-packages/sphinx/util/pycompat.py", line 81, in execfile_
    exec(code, _globals)
  File "/data/mhvk/packages/baseband/docs/conf.py", line 162, in <module>
    if versionmod.release:
AttributeError: module 'baseband.version' has no attribute 'release'

Also in the cookiecutter template, there seems an expectation of a version.release attribute, inside the edit_on_github part.

pllim commented 4 years ago

AttributeError: module 'baseband.version' has no attribute 'release'

The fix:

from pkg_resources import get_distribution

# The full version, including alpha/beta/rc tags.
release = get_distribution(project).version
# The short X.Y version.
version = '.'.join(release.split('.')[:2])

In my review, I asked for this to be added to the guide, but @astrofrog wanted to save it for a separate clean-up PR.

mhvk commented 4 years ago

@pllim - should that go in conf.py? Or rather in the VERSION_TEMPLATE inside setup.py?

pllim commented 4 years ago

@mhvk , that should be inside docs/conf.py. Sorry for the confusion!

mhvk commented 4 years ago

Sorry, getting more confused: there are already definitions of release and version inside docs/conf.py - those should be ignored here?

https://github.com/astrofrog/package-template/blob/ef9573da29fc3b7ee7e296d308da09dabaf7993d/docs/conf.py#L50-L53

mhvk commented 4 years ago

I'm actually confused - that conf.py I linked to looks very substantially different from the one cookiecutter made. I better check more carefully....

mhvk commented 4 years ago

OK, I just looked at the wrong file: https://github.com/astrofrog/package-template/blob/ef9573da29fc3b7ee7e296d308da09dabaf7993d/docs/conf.py#L50-L53

Funny enough the last commit is for "fix edit on github" :smile:

pllim commented 4 years ago

@mhvk , now I am confused. Did you end up having to patch with https://github.com/astropy/package-template/pull/438#issuecomment-579969637 or everything works from the template?

mhvk commented 4 years ago

@pllim - I was confused too, as I was looking at the wrong file. I was wondering about your patch since that defines release and version, but those are already defined in conf.py; what is missing is a version.release. So, to apply your patch, do I just let that replace the version and release that are in conf.py and then use those instead of versionmod.release? (latter at https://github.com/astrofrog/package-template/blob/ef9573da29fc3b7ee7e296d308da09dabaf7993d/%7B%7B%20cookiecutter.package_name%20%7D%7D/docs/conf.py#L159-L170)

pllim commented 4 years ago

@mhvk ... Oh, I get it now. For that section in particular, maybe you should only provide the "master" option if you follow the same release procedures as astropy core library. See astropy/astropy#9379

I don't use "edit on GitHub" for my stuff, so my patch is probably irrelevant to that logic block. Sorry for the confusion! :see_no_evil:

mhvk commented 4 years ago

@astrofrog - separately, in recommending tox, it is perhaps good for people to realize that this means you get a .tox subdirectory for every package you have that will contain GBs of data. It would be nice to isolate this from the package directory (I'll see if I can find the relevant environment variables or so). Similarly, any temporary files are now stored in .tmp rather than on a /tmp partition, ie., more stuff to clean up oneself (of course, only a git clean away).

bsipocz commented 4 years ago

@mhvk - thanks for digging up these issues.

mhvk commented 4 years ago

@pllim - yes, that makes sense. Much better to always point to master. since if things have already changed, you really want people to notice before doing work!

mhvk commented 4 years ago

Another issue with tests is that any files created in the doctests are just put wherever pytest is run. But I now see that that is just https://github.com/astropy/pytest-doctestplus/issues/68 - would be nice to address that!

mhvk commented 4 years ago

Just to summarize, since I left rather many comments: the real ones are in-line:

astrofrog commented 4 years ago

@mhvk - thanks for the careful review!

Regarding the 'release and version in conf.py, the latest version in the present conf.py in the template should work nicely:

# The short X.Y version.
version = package.__version__.split('-', 1)[0]
# The full version, including alpha/beta/rc tags.
release = package.__version__

or alternatively what @pllim said, but indeed I'd like to wait until a follow up PR for that since there's nothing actually wrong with the above approach and in fact it can be faster than using get_distribution. But either way the important thing is that release is something that is only defined in conf.py, not something the package/version.py file would ever provide.

Regarding conftest.py, things are a little trickier. The issue with moving conftest.py to the root is that it does not get installed, so then running packagename.test() in an interactive Python session won't work. In fact, the cleanest approach here is to leave conftest.py inside the package but then to also duplicate the list of packages to show in the setup.cfg file:

[tool:pytest]
...
astropy_header = true
astropy_header_packages =
    scipy
    matplotlib

In general I agree that this is not ideal and really moving conftest.py to the root of the package would be the 'proper' way to do it and then not supporting running self-tests inside installed packages (i.e. the test() command). But I'm pretty sure a number of people feel strongly about keeping that.

One alternative I want to put out there is to simply get rid of the list of dependencies in the pytest header and instead just rely on commands like pip freeze or conda list if people need to check installed packages. In fact that's why I added pip freeze as a command inside the tox config, because that gives a much more complete picture of things. This is my personal preference - to keep the custom header for the other bits but to not actually show the package list.

My second favorite option is to actually just generate that list of dependencies automatically from the declared package dependencies, as I suggested in https://github.com/astropy/astropy/issues/8867 - that idea met a lot of opposition but maybe it would be a good time to reconsider. This would simplify things a lot since there would be no need to override the options in either conftest.py or setup.py.

mhvk commented 4 years ago

@astrofrog - I agree that the lines in conf.py are fine, but those are not used in edit_on_github:

https://github.com/astrofrog/package-template/blob/970fe945438539c1cfb0306088639e5840c75b25/%7B%7B%20cookiecutter.package_name%20%7D%7D/docs/conf.py#L159-L170

That said, I think that @pllim's solution of just setting the relevant branch to master is better anyway!

mhvk commented 4 years ago

On the list of dependencies, having looked back at the thread, I think the consensus was that there should be a way to specify it manually, but I didn't see a big push against having an automatic default that made sense. To me, the best default would be explicit dependencies mentioned in setup.cfg, but that really doesn't matter that much; it could also be something that is a special key, say astropy_header_packages = 'requierd' and maybe 'recursive'.

A propos package/conftest.py, could that be generated upon installation like version.py?

p.s. It may be smartest to move this to a post-this-PR discussion!

pllim commented 4 years ago

Oh yeah, @astrofrog , should there be a mention for those to push their releases to conda-forge about replicating pyproject.toml stuff in the recipe, or is that a separate PR? I think we talked about it on Slack.

bsipocz commented 4 years ago

In general I agree that this is not ideal and really moving conftest.py to the root of the package would be the 'proper' way to do it and then not supporting running self-tests inside installed packages (i.e. the test() command). But I'm pretty sure a number of people feel strongly about keeping that.

Very strongly indeed. But mine are not as strong as @eteq's, so if you convince him I could come on board, too. It proved to be super useful to test user installs, etc. and as I recall was one of the requirement to make work before removing the helpers.

bsipocz commented 4 years ago

One alternative I want to put out there is to simply get rid of the list of dependencies in the pytest header and instead just rely on commands like pip freeze or conda list if people need to check installed packages. In fact that's why I added pip freeze as a command inside the tox config, because that gives a much more complete picture of things. This is my personal preference - to keep the custom header for the other bits but to not actually show the package list.

Please keep the list there. I use it all the time, both locally and CI. It's convenient, and also accurately points to the versions that are actually being used, while pip freeze etc can be tricked with paths etc. to show something else than the actual python interpreter sees.

I'm more flexible with the full list, but as we already talked before, populating it with irrelevant stuff adds noise, while could miss important but assumed dependencies. Comparing builds using the header is also super quick, but it again needs that the list should be the same, and should thus list when something is not installed in one, but it is in the other.

mhvk commented 4 years ago

Having done a second package now, which has more of a mix of relevant and less relevant dependencies, my preference would be for the ability to provide a list to be in setup.cfg using the astropy_header_packages entry (which is already possible and works well), and then have the install generate package/conftest.py with the appropriate packages scraped together).

astrofrog commented 4 years ago

@pllim @mhvk @bsipocz - I've pushed a few more changes:

For the issue of tox not picking up the conftest.py file in time, I'd like to defer this to a follow up PR if possible. I agree that the solution of specifying the packages in setup.cfg and somehow having that information be copied into the installed version would be nice, but I'd like to investigate that in a dedicated PR. I've opened an issue to keep track of this: https://github.com/astropy/package-template/issues/439 - so can we ignore it for the purposes of the PR and consider it an ongoing bug?

Otherwise I think I've addressed all the comments so far, but let me know if I missed something!

mhvk commented 4 years ago

I don't think I'll have time for another detailed look today, so would suggest to just merge this - it was already great as it was and clearly is now even better! And we have follow-up issues raised already.

Super-nice work!

pllim commented 4 years ago

Merged. Thank you, all!