APE 17 migration #1674

epassaro · 2021-06-28T21:27:11Z

Description

Adoption of the new Astropy package template: https://docs.astropy.org/projects/package-template/en/latest/ape17.html

What's working?

Package installation ⚠️ requires releasing manually a new compatible tag to work
Unit Tests
Documentation Build: https://epassaro.github.io/tardis/branch/ape-17-migration/installation.html#requirements
Compare Reference Data (requires package installation)
Scripts/Entry points
CalVer support :)

Suggestions?

Full list of changes

.github/workflows/documentation-build.yml - use "new" doc build command (deprecated python setup.py build_docs)
.github/workflows/documentation-preview.yml - same as above
.gitignore - re-rendered with cookiecutter and replaced
.gitmodules - removed, astropy_helpers deprecated :)
MANIFEST.in - re-rendered
README.md - merged with file rendered with cookiecutter
ah_bootstrap.py - removed
astropy_helpers - removed
docs/Makefile - re-rendered
docs/conf.py - merged
docs/make.bat - re-rendered
ez_setup.py - removed
licenses/LICENSE.rst - moved to new licenses folder
licenses/README.rst - new
licenses/TEMPLATE_LICENCE.rst - new
pyproject.toml - merged
setup.cfg - re-rendered, added entry_points, package_data and disabled doctest
setup.py - re-rendered, tweaked to make CalVer work
tardis/__init__.py - merged
tardis/__astropy_init.py - re-rendered
tardis/conftest.py - merged
tardis/io/setup_package.py - APE 17 step 4
tardis/montecarlo/setup_package.py - APE 17 step 4
tardis/plasma/setup_package.py - APE 17 step 4
tardis/simulation/setup_package.py - APE 17 step 4
scripts/tardis → tardis/scripts/tardis.py - moved to make it work as an entry point
tardis/tests/integration_tests/conftest.py - githash not available in new version system (setuptools_scm)
tardis/tests/integration_tests/plot_helpers.py - same as above
tardis/tests/integration_tests/report.py - same as above
tardis/tests/setup_package.py - APE17 step 4
tardis_env3.yml - add required setuptools_scm and sphinx-astropy
tox.ini - new from cookiecutter

Coverage report
Moving tardis script to tardis/scripts makes coverage fail (we have more code lines in PR than in the base branch)

Motivation and context

Open issues:
- Can't install TARDIS with pip #1211
- Update TARDIS setup and package infrastructure #1216
Getting rid of troublesome astropy_helpers submodule
Update conda-forge package

How has this been tested?

Testing pipeline.
Other.

Examples

Type of change

Bug fix.
New feature.
Breaking change.
None of the above.

Checklist

My change requires a change to the documentation.
- I have updated the documentation accordingly.
- (optional) I have built the documentation on my fork following the instructions.
I have assigned and requested two reviewers for this pull request.

codecov · 2021-06-28T22:45:50Z

Codecov Report

Merging #1674 (0534580) into master (d5ac38a) will decrease coverage by 0.57%.
The diff coverage is n/a.

❗ Current head 0534580 differs from pull request most recent head 88a5869. Consider uploading reports for the commit 88a5869 to get more accurate results

@@            Coverage Diff             @@
##           master    #1674      +/-   ##
==========================================
- Coverage   61.86%   61.29%   -0.58%     
==========================================
  Files          62       63       +1     
  Lines        5738     5792      +54     
==========================================
  Hits         3550     3550              
- Misses       2188     2242      +54

Impacted Files	Coverage Δ
tardis/tardis/scripts/tardis.py	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5ac38a...88a5869. Read the comment docs.

epassaro · 2021-06-29T00:59:04Z

/azp run compare-refdata

azure-pipelines · 2021-06-29T00:59:14Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2021-06-29T01:03:28Z

Build failed 792eaa2

Click here to see results.

tardis-bot · 2021-06-29T22:17:49Z

Before a pull request is accepted, it must meet the following criteria:

Is the necessary information provided?
Is this a duplicate PR?
- If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
- If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
Does it pass existing tests and are new tests provided if required?
- The test coverage should not decrease, and for new features should be close to 100%.
Is the code tidy?
- No unnecessary print lines or code comments.

andrewfullard

Some minor comments, otherwise looks good

andrewfullard · 2021-07-01T17:39:09Z

licenses/LICENSE.rst

@@ -0,0 +1,26 @@
+Copyright 2021 TARDIS Collaboration


Static dates seem like a bad idea?

Didn't see that. TARDIS is 2014-present or 2013-present? @wkerzendorf

andrewfullard · 2021-07-01T18:04:24Z

tardis/tests/integration_tests/plot_helpers.py

@@ -2,7 +2,8 @@
 import tempfile

 from pytest_html import extras
-from tardis import __githash__ as tardis_githash
+
+# from tardis import __githash__ as tardis_githash


Why comment and not just delete?

Someone will need to fix integration tests in the future and maybe that line would be useful. I was thinking of adding a comment like not available in version.py anymore, find another way! or something like that.

andrewfullard · 2021-07-01T18:07:29Z

tardis/tests/integration_tests/report.py

@@ -33,7 +33,7 @@
 # For specifying error while exception handling
 from socket import gaierror

-from tardis import __githash__ as tardis_githash
+# from tardis import __githash__ as tardis_githash


Why comment?

wkerzendorf · 2021-07-01T21:15:36Z

tardis/scripts/tardis.py

+    "compiling.",
+)
+
+parser.add_argument(


@KevinCawley what is that?

I'm sorry I don't follow.

* remove ah submodule * re-render setup.cfg * remove get_package_data functions * remove ah submodule (2) * remove ez_setup.py * re-render setup.py * update pyproject.toml * re-render .gitignore * re-render MANIFEST.in * add tox.ini * re-render _astropy_init.py * update __init__.py * update conftest.py * add licenses folder * add license section to readme * re-render docs cfg * comment out __githash__ imports * remove plasma setup_package.py * disable doctests * re apply black format * fix author name * add required dependency * first round of changes to conf.py * add jaladh changes to conf.py * changes to doc workflows [build_docs] * add atharva patch * fix layout * include package data [build_docs] * add missing nbsphinx config * more changes to conf.py [build_docs] * minor changes * add entry points * black format script * minor changes * add support for CalVer :) * comment out calver cfg * fix typo * minor changes * consistent use of quotes in conf.py * fix copyright [build_docs] * remove obsolete .nojekyll from docs folder * fix year in license * add comment about missing __githash__ * [build_docs]

epassaro marked this pull request as draft June 28, 2021 21:27

epassaro force-pushed the ape-17-migration branch 2 times, most recently from 8789dae to 6af97b8 Compare June 28, 2021 23:53

epassaro force-pushed the ape-17-migration branch 2 times, most recently from 78bb0f5 to 08ecf86 Compare June 29, 2021 19:00

epassaro requested review from wkerzendorf and andrewfullard June 29, 2021 22:17

epassaro marked this pull request as ready for review June 29, 2021 22:47

epassaro added bugfix 🪲 priority - high 🔥 CI/CD install labels Jun 30, 2021

epassaro force-pushed the ape-17-migration branch from c809c6b to 4d4a3c8 Compare June 30, 2021 21:18

andrewfullard reviewed Jul 1, 2021

View reviewed changes

epassaro force-pushed the ape-17-migration branch from 05bf56d to 34381dd Compare July 1, 2021 18:36

jaladh-singhal force-pushed the master branch 2 times, most recently from 2907807 to d5ac38a Compare July 1, 2021 18:47

epassaro added 8 commits July 1, 2021 17:59

remove ah submodule

cab1a55

re-render setup.cfg

3e5c353

remove get_package_data functions

e628a22

remove ah submodule (2)

cbde465

remove ez_setup.py

97ce24f

re-render setup.py

df7570c

update pyproject.toml

6b8b9d3

re-render .gitignore

31a81c0

epassaro added 19 commits July 1, 2021 17:59

add required dependency

de41ff0

first round of changes to conf.py

a06580d

add jaladh changes to conf.py

8a07b7f

changes to doc workflows [build_docs]

82bbdb9

add atharva patch

c507350

fix layout

e27679c

include package data [build_docs]

282a5bc

add missing nbsphinx config

68289f5

more changes to conf.py [build_docs]

3bda96c

minor changes

f529e93

add entry points

1d93392

black format script

a1915c5

minor changes

167421f

add support for CalVer :)

368e342

comment out calver cfg

b5b4a4e

fix typo

04a18a6

minor changes

47e62d0

consistent use of quotes in conf.py

62724df

fix copyright [build_docs]

4af2443

epassaro force-pushed the ape-17-migration branch from 06eb500 to 4af2443 Compare July 1, 2021 20:59

epassaro added 2 commits July 1, 2021 18:04

remove obsolete .nojekyll from docs folder

c748096

fix year in license

f65f45f

wkerzendorf reviewed Jul 1, 2021

View reviewed changes

epassaro added 2 commits July 1, 2021 18:46

add comment about missing __githash__

29ddcce

[build_docs]

88a5869

wkerzendorf merged commit 01a3495 into tardis-sn:master Jul 1, 2021

This was referenced Jul 2, 2021

fix pip install #1686

Merged

Update TARDIS setup and package infrastructure #1216

Closed

jaladh-singhal mentioned this pull request Jul 2, 2021

Improve docs layout #1687

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APE 17 migration #1674

APE 17 migration #1674

epassaro commented Jun 28, 2021 •

edited

Loading

codecov bot commented Jun 28, 2021 •

edited

Loading

epassaro commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

tardis-bot commented Jun 29, 2021

andrewfullard left a comment

andrewfullard Jul 1, 2021

epassaro Jul 1, 2021

epassaro Jul 1, 2021

andrewfullard Jul 1, 2021

epassaro Jul 1, 2021

andrewfullard Jul 1, 2021

wkerzendorf Jul 1, 2021

KevinCawley Jul 2, 2021 •

edited

Loading

APE 17 migration #1674

APE 17 migration #1674

Conversation

epassaro commented Jun 28, 2021 • edited Loading

codecov bot commented Jun 28, 2021 • edited Loading

Codecov Report

epassaro commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

tardis-bot commented Jun 29, 2021

andrewfullard left a comment

Choose a reason for hiding this comment

andrewfullard Jul 1, 2021

Choose a reason for hiding this comment

epassaro Jul 1, 2021

Choose a reason for hiding this comment

epassaro Jul 1, 2021

Choose a reason for hiding this comment

andrewfullard Jul 1, 2021

Choose a reason for hiding this comment

epassaro Jul 1, 2021

Choose a reason for hiding this comment

andrewfullard Jul 1, 2021

Choose a reason for hiding this comment

wkerzendorf Jul 1, 2021

Choose a reason for hiding this comment

KevinCawley Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

epassaro commented Jun 28, 2021 •

edited

Loading

codecov bot commented Jun 28, 2021 •

edited

Loading

KevinCawley Jul 2, 2021 •

edited

Loading