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

Add workflow to publish to pypi #62

Merged
merged 18 commits into from
May 21, 2020
Merged

Add workflow to publish to pypi #62

merged 18 commits into from
May 21, 2020

Conversation

steven-murray
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #62 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   65.41%   65.52%   +0.11%     
==========================================
  Files          22       23       +1     
  Lines        2495     2509      +14     
==========================================
+ Hits         1632     1644      +12     
- Misses        863      865       +2     
Impacted Files Coverage Δ
src/hmf/__init__.py 85.71% <ø> (ø)
src/hmf/_internals/_cache.py 85.56% <ø> (ø)
src/hmf/_internals/_framework.py 89.41% <ø> (ø)
src/hmf/_internals/_utils.py 36.36% <ø> (ø)
src/hmf/alternatives/wdm.py 92.66% <ø> (ø)
src/hmf/cosmology/__init__.py 100.00% <ø> (ø)
src/hmf/cosmology/growth_factor.py 78.04% <ø> (ø)
src/hmf/density_field/__init__.py 100.00% <ø> (ø)
src/hmf/density_field/filters.py 68.05% <ø> (ø)
src/hmf/density_field/halofit.py 82.85% <ø> (ø)
... and 14 more

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 a218f2a...c0a796e. Read the comment docs.

@webknjaz
Copy link

I checked out this branch and found out that adding use_scm_version = True to the setuptools.setup() invocation helps: pypa/setuptools-scm#414 (comment).

P.S. Why don't you use pep517package as shown in my tutorial? This would probably work w/o changes to setup.py.
P.P.S. You have code in setup.py, it's generally discouraged, you can migrate the metadata to setup.cfg and keep only import setuptools; setuptools.setup() (yes, just that!) in setup.py. Remember, it's not a Makefile. It's common to use tox/invoke/nox/make for automations like building, testing and publishing it's then easier to map that to CIs.

@steven-murray
Copy link
Collaborator Author

@webknjaz Thanks for helping out here. I probably should move to setup.cfg -- I have never really gotten the benefit of it over just putting the values into setup.py, but if it helps, I will!

I did actually start by trying pep517.build but it was giving me errors (something about missing parameters) and I couldn't find the relevant documentation to sort out the errors. That would be nicer though!

@webknjaz
Copy link

Ah, I see. Looking at the log, it's noticeable that it complains about the unspecified PEP 517 build backend: https://github.com/steven-murray/hmf/runs/694053891#step:6:19.

When using setuptools, just add build-backend = "setuptools.build_meta" under [build-system] in pyproject.toml: https://github.com/sanitizers/octomachinery/blob/master/pyproject.toml#L8.

This basically tells the build frontend tool (pip/pep517/flit/poetry/etc.) what it should import and call in order to build the project: https://www.python.org/dev/peps/pep-0517/#source-trees

@@ -2,3 +2,4 @@
requires = ["setuptools>=42", "wheel", "setuptools_scm[toml]>=3.4"]

Choose a reason for hiding this comment

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

Suggested change
requires = ["setuptools>=42", "wheel", "setuptools_scm[toml]>=3.4"]
requires = ["setuptools>=42", "wheel", "setuptools_scm[toml]>=3.4"]
build-backend = "setuptools.build_meta"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup. I found that 🤦

Thanks!

Choose a reason for hiding this comment

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

The thing is that some frontends (like pip) implement a fallback and set it to setuptools.build_meta.__legacy__ and others don't.

@steven-murray
Copy link
Collaborator Author

@webknjaz Two things:

First -- thanks for the very easy to follow documentation here.

Second, if you have a moment to answer me -- in those instructions, the actual PyPI upload is only done when a tag is commited. I wonder how this works -- if I push a tag to a branch which is on a PR, will that commit also be uploaded to PyPI, even before I know if it's passed tests etc and merged?

@steven-murray steven-murray merged commit 0333314 into master May 21, 2020
@steven-murray steven-murray deleted the pypi branch May 21, 2020 04:07
@@ -0,0 +1 @@
!coverage.py: This is a private format, don't read it directly!{"lines":{"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/__init__.py":[1,2,3,4,6,7,12,13,14,15,16,17],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/alternatives/__init__.py":[4],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/alternatives/wdm.py":[8,10,11,12,13,14,15,17,23,47,49,63,83,105,107,109,114,129,138,152,162,163,166,184,186,50,51,57,59,61,192,196,212,214,216,220,236,238,240,244,260,262,264,273,284,287,297,313,322,338,349,356,368,370,376,394,401],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/density_field/__init__.py":[4,5,6],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/density_field/transfer.py":[7,8,9,10,11,12,13,15,16,18,23,44,48,49,50,51,52,53,54,55,56,57,58,59,94,110,119,140,149,160,173,182,191,200,209,231,236,243,250,257,270,275,282,287,292,297,305,310,317,326],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/_internals/__init__.py":[3],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/_internals/_cache.py":[8,9,10,13,21,131,138,175,262,176,178,243,245,256,257,260,50,52,112,114,128,180,18,183,187,194,195,196,197,199,201,246,247,250,203,205,206,209,211,213,216,217,218,219,222,132,133,224,229,233,235,188,253,54,57,58,59,60,65,70,74,84,85,87,90,93,251,94,97,98,101,102,103,105,108,110,202,66,67,71,225,236,75,76,79,81],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/density_field/halofit.py":[8,9,10,11,12,13,14,17,106],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/cosmology/__init__.py":[3,4,5],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/cosmology/cosmo.py":[12,14,15,16,17,20,41,43,51,69,90,98,110,45,48,59,60,119,120,49,85,96,62,67,104,105],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/_internals/_framework.py":[3,4,5,11,23,25,27,41,56,74,88,90,93,106,112,113,133,141,152,158,184,185,29,37,38,91,98,99,100,101,103],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/cosmology/growth_factor.py":[8,10,11,12,13,14,15,17,18,20,25,28,30,35,55,56,58,66,106,122,149,165,189,190,205,207,209,216,225,271,298,299,316,318,320,337,354,380,382,383,389,391,430],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/_internals/_utils.py":[1,4,5,13],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/density_field/transfer_models.py":[6,7,8,9,11,12,13,14,16,17,19,23,26,39,41,43,47,64,82,84,86,112,139,141,157,159,161,206,233,320,326,344,346,348,380,393,395,399,478,482,494,541,554,556,564,594,620,621,623,661,684,685,687,712,713,715],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/density_field/filters.py":[4,6,7,8,9,10,11,14,57,59,65,79,95,118,141,159,193,206,220,257,272,273,299,301,305,308,311,314,322,323,349,351,354,357,360,363,367,368,399,400,402,406,409,412,417,422,425,451,452,459,461,463,477,483,489,496,503,512,515,522,528,534],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/mass_function/__init__.py":[3,4],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/mass_function/hmf.py":[6,8,9,10,11,12,13,15,16,17,18,19,20,22,25,66,70,71,72,73,74,75,76,77,78,79,100,109,118,127,141,150,169,185,194,210,219,224,232,246,251,256,261,266,271,284,289,296,328,333,349,356,407,414,421,465,479,493,514],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/mass_function/fitting_functions.py":[6,8,9,10,11,12,13,14,15,18,63,73,74,75,76,77,78,79,80,81,113,137,177,203,204,205,208,210,213,224,305,310,315,320,325,333,339,341,342,344,345,347,122,132,133,349,354,356,357,359,360,361,363,365,366,367,368,369,370,371,372,373,374,375,376,377,378,379,380,85,86,87,88,89,90,91,92,94,95,96,97,98,99,101,104,105,106,107,108,109,110,383,397,405,408,410,413,415,417,418,419,420,422,423,424,425,426,427,428,429,430,431,432,433,434,435,438,439,442,446,454,456,457,459,460,461,463,465,466,467,468,469,470,471,472,473,475,476,477,478,479,480,481,482,483,484,485,486,488,489,490,491,492,493,494,497,507,512,514,516,517,518,520,522,523,524,525,526,527,528,529,530,531,532,533,534,535,536,537,540,547,552,553,554,556,557,558,560,562,563,565,566,567,568,569,570,571,572,573,574,575,576,577,579,580,581,582,583,585,586,587,588,589,590,591,592,593,594,595,596,597,599,600,601,602,603,604,605,608,629,634,635,636,638,639,640,641,643,644,646,660,665,666,667,668,669,670,672,673,674,675,676,677,678,679,680,681,682,683,684,685,686,687,690,699,704,705,706,709,710,712,713,714,716,717,718,719,720,721,722,723,724,725,726,727,728,729,730,731,734,739,740,741,742,744,745,746,748,749,750,751,754,755,756,757,758,759,760,761,762,763,764,765,766,767,768,769,770,771,772,773,774,775,776,777,780,809,844,849,850,852,853,855,856,857,858,859,860,861,862,863,866,867,868,869,870,871,872,873,874,875,876,877,878,879,880,881,884,892,897,898,899,900,901,903,904,905,906,907,908,909,910,911,912,913,914,915,916,917,918,921,926,927,928,930,931,932,934,935,936,937,938,939,942,943,944,945,946,947,948,949,950,951,952,953,954,955,956,958,959,960,961,965,970,988,993,994,995,997,998,999,1001,1003,1004,1005,1006,1007,1008,1009,1010,1011,1012,1013,1014,1015,1016,1017,1018,1019,1020,1023,1024,1025,1026,1027,1028,1029,1030,1031,1032,1033,1034,1035,1036,1037,1038,1039,1040,1042,1044,1045,1046,1047,1048,1049,1050,1051,1052,1053,1054,1055,1056,1057,1058,1059,1060,1061,1064,1065,1066,1067,1068,1069,1070,1071,1072,1073,1074,1075,1076,1077,1078,1079,1080,1081,1083,1084,1086,1087,1088,1089,1090,1091,1092,1093,1094,1095,1096,1097,1098,1099,1100,1101,1102,1103,1105,1107,1108,1109,1110,1111,1112,1113,1114,1115,1116,1117,1118,1119,1120,1121,1122,1123,1124,1126,1127,1128,1129,1132,1133,1134,1135,1136,1137,1138,1139,1140,1141,1142,1143,1144,1145,1146,1147,1148,1149,1152,1153,1154,1155,1156,1157,1158,1159,1160,1161,1162,1163,1164,1165,1166,1167,1168,1169,1170,1173,1174,1175,1176,1177,1178,1179,1180,1181,1182,1183,1184,1185,1186,1187,1188,1189,1190,1193,1194,1195,1196,1197,1198,1199,1200,1201,1202,1203,1204,1205,1206,1207,1208,1209,1210,1216,1217,1218,1219,1220,1221,1222,1223,1224,1226,1227,1228,1229,1230,1231,1232,1233,1234,1236,1237,1238,1239,1240,1241,1242,1243,1244,1246,1247,1248,1249,1250,1251,1252,1253,1254,1256,1257,1260,1262,1307,1315,1327,1328,1329,1331,1332,1333,1335,1338,1339,1340,1341,1342,1343,1344,1345,1346,1348,1349,1350,1351,1352,1353,1354,1355,1356,1358,1359,1360,1361,1362,1363,1364,1365,1366,1368,1369,1370,1371,1372,1373,1374,1375,1376,1378,1379,1380,1381,1382,1383,1384,1385,1386,1388,1389,1390,1391,1392,1395,1396,1398,1483,1500,1510,1522,1523,1535,1536,1537,1540,1541,1542,1543,1544,1545,1546,1547,1548,1549,1550,1551,1552,1553,1554,1556,1557,1558,1559,1563,1584,1585,1586,1587,1589,1591,1592,1593,1594,1595,1596,1597,1598,1599,1600,1601,1602,1603,1604,1605,1607,1608,1610,1611,1616,1617,1618,1620,1622,1623,1624,1625,1626,1627,1628,1629,1630,1631,1632,1633,1634,1635,1636,1638,1639,1640,1641,1646,1647,1648,1649,1651,1653,1654,1655,1656,1657,1658,1659,1660,1661,1662,1663,1664,1665,1666,1667,1669,1670,1671,1672,1676],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/halos/__init__.py":[4],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/halos/mass_definitions.py":[5,6,7,8,9,10,13,14,15,16,17,18,19,23,36,38,55,66,86,106,110,199,200,202,205,206,208,210,216,217,219,221,227,231,233,241,242,244,246,265,326,327],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/mass_function/integrate_hmf.py":[3,4,5,6,9,10,13],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/helpers/__init__.py":[3,5],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/helpers/functional.py":[11,13,14,15,18,83,84,86,87,88,89,90,91,92,224],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/helpers/sample.py":[5,6,7,8,11,19,28,71],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/fitting/cli_tools.py":[],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/fitting/fit.py":[],"/home/steven/Documents/Projects/halos/HMF/hmf/src/hmf/fitting/__init__.py":[]}}

Choose a reason for hiding this comment

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

You should add such files to gitignore instead of comitting.

Comment on lines +21 to +22
- name: Set up Python 3.7
uses: actions/setup-python@v1

Choose a reason for hiding this comment

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

FYI this action is already v2.

- name: Install pep517
run: |
python -m pip install pep517 --user
python setup.py --version

Choose a reason for hiding this comment

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

could also be python -m setuptools_scm ls

- repo: https://github.com/pre-commit/pygrep-hooks
rev: v1.5.1
hooks:
- id: rst-backticks

Choose a reason for hiding this comment

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

FYI you may also like this hook that Anthony has: https://github.com/asottile/setup-cfg-fmt#as-a-pre-commit-hook

@@ -15,7 +15,7 @@ import sys
import os
import traceback

import hmf
from src import hmf

Choose a reason for hiding this comment

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

This looks wrong...

author-email = steven.g.murray@asu.edu
license = mit
long-description = file: README.rst
long-description-content-type = text/x-rst; charset=UTF-8

Choose a reason for hiding this comment

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

This attr doesn't have a kebab-case alias like some of the others: ee4b1d7#r39336456 / https://setuptools.readthedocs.io/en/latest/setuptools.html#metadata.

mpmath>=1.0.0
colossus>=1.2.1

;[options.entry_points]

Choose a reason for hiding this comment

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

It is recommended to use console_scripts entrypoint instead of standalone in-repo scripts like you used to.

@@ -8,8 +8,8 @@
import os
import sys

from hmf import MassFunction
from hmf import fit
from src.hmf import MassFunction

Choose a reason for hiding this comment

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

This completely defeats the purpose of having src-layout in the repo. Here, read some explanation: https://blog.ganssle.io/articles/2019/08/test-as-installed.html / https://hynek.me/articles/testing-packaging/.

@webknjaz
Copy link

@steven-murray I've added a few additional review comments on how to fix some of the broken things.

Second, if you have a moment to answer me -- in those instructions, the actual PyPI upload is only done when a tag is commited. I wonder how this works -- if I push a tag to a branch which is on a PR, will that commit also be uploaded to PyPI, even before I know if it's passed tests etc and merged?

GitHub has a system of internal events that get published to all the listeners (pre-repo webhook integrations, GitHub Apps, GitHub Actions that are actually built on top of GitHub Apps, and so on). By adding a GHA workflow to your repo you subscribe to some events that you specify there. In your case, it's push.

Now, this even happens whenever somebody does git push and GHA (among others) will receive an event and trigger a workflow(s). It's important to understand that this event will hit your workflow if it happens inside of your repo. Meaning that this git push should be done from the inside. It doesn't matter if it's related to a branch or a tag, both would equally produce this push event.

There's also a pull_request event that happens when somebody interacts with PRs. That's why you can often see a double amount of jobs in your PRs when the PR branch belongs to your repo — two events will happen on the same commit: push and pull_request. And this will trigger two separate workflows (or more, depending on your setup, of course).

When somebody forks your repo and pushes things there, your workflows won't receive those events. But if they open a PR against your repo, you'll get pull_request events triggering your workflows but no push events. This is because GHA is scoped to only access your things.

If you have multiple workflows, they will be triggered simultaneously. There's no way to say that one workflow depends on another. This is handy when you want to have something like linters run in parallel but not preventing your tests from running on linter violations.

OTOH inside a workflow, you may have multiple jobs and you can set up dependencies between them, using the needs setting.

Finally, within a job, each step depends on the previous one by default, unless changed with things like if: failure().

What does this leave us with? In order to ensure that your project is tested before publishing, you should put it in the same workflow as your tests and declare a needs relation. You can customize the publish conditions, using if setting on the step of job level.

You could also make sure to wait for the tests on your target commit manually and only then push a tag, this will generate a separate workflow run anyway. Some people switch from git push origin v1.0.0 to using a different event like release (it's triggered by creating a release on the GitHub UI).
Besides, there's also a possibility to have your very own custom event using repository_dispatch but triggering it requires hitting GitHub's API yourself so it may seem slightly harder.

I myself, like the idea of building the dists, then testing those dists (not the source in the project dir!), and only then publishing them to PyPI (those same dists from the step 1, instead of building them again w/o any guarantee that they will work the same way as the source you tested).
I have a state-of-the-art implementation of this idea @ https://github.com/ansible/pylibssh/blob/devel/.github/workflows/build-test-n-publish.yml.

Have fun :)

@steven-murray
Copy link
Collaborator Author

@webknjaz Thanks so much for that detailed response. I'm still getting used to what Github really means by "push" and "pull_request" etc (as in, specifically what events trigger what things), and that helped a lot.

While playing around I kind of arrived at this solution: test on every push/PR and also publish to Test PyPI on every push, then on pushes to master (i.e. merged PRs, since I have protections on master) I run an automatic semantic version tagger, then since that makes a push, I have another workflow which runs only on pushes that create a tag, which publishes to actual PyPI.

Coming to think of it, your example in the tutorial probably already does this for free with its if: statement (though I didn't understand the function that it uses there, so I was hesitant).

I'll also have a closer look through your state of the art implementation and try stealing some ideas :-)

Thanks again for all your help!

@webknjaz
Copy link

run an automatic semantic version tagger, then since that makes a push, I have another workflow which runs only on pushes that create a tag, which publishes to actual PyPI.

That only works if you don't use the built-in secrets.GITHUB_TOKEN because this one doesn't produce events.

I'll also have a closer look through your state of the art implementation and try stealing some ideas :-)

Have fun :) Just beware that it's probably more complicated than you need because it has to build like 20 OS-specific wheels which makes the matrix big.

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.

2 participants