Feature/initial GitHub action

[jagerber48]

Continuation of #191. Making the PR to get github actions going from a local rather than forked branch.

[wshanks]

Carrying over from #191:

If I include windows and mac in the matrix in parallel then there are 12 cases. Feels like kind of a lot, but what are there downsides to having too many test configurations?

Yes, 12 is a lot. At the moment, every run finishes in less than a minute so it's not a big deal. With slower CI runs, I usually cut down to just oldest and newest Python versions (so 3.8 and 3.12) and only run the full set of versions before release or on the main branch on a schedule rather than on every push to a PR.

One downside to a lot of builds is that GitHub has concurrency limits, but they're 20 jobs total and 5 Mac jobs, so currently a single run of the matrix is under the limit and jobs shouldn't be getting queued. Also, it's not so bad since the CI is quick. For slower CI, there is more chance that multiple PRs could get pushes around the same time and one PR's jobs would be queued behind the other. Another downside is that any flakiness gets amplified -- there is 12 times the chance of an error versus running the tests in only one environment. Hopefully this isn't much of an issue -- you just re-run the failed job. It gets annoying with slower CI, especially when using auto-merge where you come back and find something you thought would have merged didn't because one job failed for something like pip hitting a fluke networking error downloading a dependency. The final downside I can think of is just the environment -- using a bit more CPU cycles than may be necessary.

For some reason the failing appveyer check went away.

Interesting. I haven't seen many open source projects using AppVeyor or Travis recently. I think they have fallen out of favor since GitHub Actions came out with a larger free tier and better GitHub integration. I think we can just remove their config files and integrations in the repo settings as part of enabling GitHub Actions here.

The AppVeyor log seems kind of flaky any way (but it gives errors for certain event types normally):

[wshanks]

This looks good to me to start out.

I don't know if we should confirm in a discussion what kind of approval process we want for PRs going forward before merging?

[andrewgsavage]

lgtm. very nice to see tests running! We can revisit which versions are run in CI if we have trouble with long queue times.

This looks good to me to start out.

I don't know if we should confirm in a discussion what kind of approval process we want for PRs going forward before merging?

Yea, a defined approval process would speed things up

[newville]

Yes, this looks good to me -- thanks! I am in favor of merging.

Two comments for going forward from here:

• I have found having separate scripts of "test_windows.yml", "test_macos.yml", "test_linux.yml" etc is a reasonable way to manage a large-ish matrix. It can also give 3 separate badges "tests pass/fail on windows", "... macos", "... linux". I agree that here (pure python, few dependencies, very fast testing), that is not a huge issue. No preference, just a suggestion.
• it would be nice to eventually (or even soon) move the testing code to a separate directory, which might make small changes to the test scripts here.

But, that is all for future (maybe near-future) work.

We have not established how to decide on what and when to merge. I vote for calling this an early win and merge as it is. I'll suggest an approach I've used with lmfit/lmfit-py:

1. we should probably have an informal policy to not merge one's own PRs without ample time (>1 week) for discussion, and to mostly discourage (but not outright prevent) that.
2. if anyone is eager to merge this right away, I am +1.
3. for this PR, I would say that if no one else merges this, and if there are no objections or further reviews, I will intend to merge this next Monday.

[jagerber48]

Ok, I went ahead and merged it. I hope that wasn't in violation of the >1 week for discussion to merge your own PR. I assumed since folks all approved there wasn't going to be further discussion. Please let me know if so. Also a question about the policy: Is it preferred to never merge your own PR if there are other people available to do so?

Also, sadly, I forgot to squash merge it so it was just a regular merge. I don't know if there's a way to correct that after the fact. I think there are settings in the repo to always force squash merges if that is what we'd like to do.

Finally one more note: This github action does not upload to codecov. It looks like at one time this repo was engaged with codecov. This may be another resource we need to get access to from @lebigot. I will open an issue for this.

[wshanks]

Regarding the squash merge -- I did a force push to squash the commits from this PR. It's not the best practice to rewrite history but the changes hadn't been on master for long. Be sure to rebase if you pulled in the last few hours.

I am adding some branch protection rules to master. These are just what I think is the ideal flow (require status checks to pass, require approval, require linear history), but we are all repo owners so we can override them if we want to.

[wshanks]

we should probably have an informal policy to not merge one's own PRs without ample time (>1 week) for discussion, and to mostly discourage (but not outright prevent) that.

I think this sounds good, and I would add that there should be some consideration of the impact of the change. Right now, we have four (or five depending on lebigot's availability) people pretty interested in the repo and can probably give quick feedback to any PR. Over time people might get pulled more on other things. A small change like updating an outdated CI setting might not need approval if no one is responsive but something that changes behavior should at least get a second opinion.

[reneeotten]

this badge in the readme results in a "404 file not found", so that doesn't seem correct

[jagerber48]

Aah, good catch. I think the branch name should be master instead of main. I'll work on this.

[reneeotten]

Regarding the squash merge -- I did a force push to squash the commits from this PR. It's not the best practice to rewrite history but the changes hadn't been on master for long. Be sure to rebase if you pulled in the last few hours.

I am adding some branch protection rules to master. These are just what I think is the ideal flow (require status checks to pass, require approval, require linear history), but we are all repo owners so we can override them if we want to.

Force-pushing to the master branch is a big no-no and should be disallowed as part of the branch protection rules.

Also squash-merge isn't always what you want. Ideally, once a PR is finalized you'd go back, clean-up the commit history in logical changes, force-push to the PR branch, and then do a "merge".

[newville]

@wshanks Thanks! I agree with you and @reneeotten that squash-merge here was a fine thing to do immediately after the non-squashed merge, but that this should be viewed as "unusual" and "rarely recommended". And thanks for adding the branch protection.

I completely agree that we have a small number (3 to 6) interested folks here, an original maintainer who has deliberately stepped back but is interested in commenting/helping. This is a perfect case to try to emphasize normal GitHub project practices of

1. no forced pushes to master unless something went really wrong.
2. branch protection on master.
3. no merging of your own PRs without a long (weeks) delay of tacit approval, thumbs-ups, etc. If it feels like other developers are not paying attention or have moved on, it might need a month or more (see package maintanence #180 for example). I should say that I have merged my own PRs at lmfit/lmfit-py on occasion, but I am the main original author and some of the PRs I make are "deep in the weeds". I feel like I do wait for feedback, but maybe others would disagree.
4. we might prefer working on branches in personal forks (as opposed to forks here in lmfit/uncertainties). I think the development pace is not so fast that forks in lmfit/uncertainties will become hard to manage.

I also agree with @reneeotten (and from our experience at lmfit/lmfit-py) that squash-merge is sometimes worse than a plain merge. Again, for a mature project with slow-ish development, I think we can make a judgment per PR, and maybe ask to requester to clean up their PR to 2 or 3 commits to hide typos and failed ideas but preserve a bit of "useful" history.

But also: these are all recommendations and not hard rules. They are mostly in place to keep everyone on an equal footing and in possession of some part of "ownership"/"responsibility". If someone is merging their own PRs without waiting for comments, and rewriting commit histories, then they are sort of expecting that others are not engaged and contributing. And that becomes self-fulfilling.

[jagerber48]

no merging of your own PRs without a long (weeks) delay of tacit approval, thumbs-ups, etc. If it feels like other developers are not paying attention or have moved on, it might need a month or more (see #180 for example).

Sorry @newville I'm having a hard time understanding what you mean here (I'm trying to learn "normal GitHub project practices" here).

Do you mean that even if one or more other maintainers have given formal or verbal approval of a PR (like in this PR) the PR author should still wait weeks before merging the PR? Is the timeline or are the conditions different if a different maintainer feels the urge to merge the PR?

[newville]

no merging of your own PRs without a long (weeks) delay of tacit approval, thumbs-ups, etc. If it feels like other developers are not paying attention or have moved on, it might need a month or more (see #180 for example).

Sorry @newville I'm having a hard time understanding what you mean here (I'm trying to learn "normal GitHub project practices" here).

I meant: avoid merging your own PRs. Well, unless it is clear that no one else is going to it.

Do you mean that even if one or more other maintainers have given formal or verbal approval of a PR (like in this PR) the PR author should still wait weeks before merging the PR? Is the timeline or are the conditions different if a different maintainer feels the urge to merge the PR?

In my opinion:

We are in a fortunate position of having 3 or more people who are knowledgeable and active enough about this code to be "maintainers": both developers who can make Pull Requests and mergers who can merge Pull Requests. So, if maintainer A makes a PR, let's wait and hope that maintainer B or C will merge that. That makes it clear that this project has a community that is working together on the code.

It's also fine to wait a few days or a week for any merge. We are not going super fast here. If you feel like people are responding too slowly or have said they agree but haven't merged, let's try to ping each other a few times.

If it is clear that no one is responding or merging to anything, then maybe you've become the sole maintainer -- this happens, sometimes for months at a time. But, if you are expecting maintainers to come back, then it is advisable to continue to go slowly, still waiting multiple days or weeks to merge PRs. If you don't do this, the project has sort of become a single-developer project. Maybe there will be outside PRs to the sole maintainer, but there will not be multiple maintainers if one person just pushing to the master/main branch continuously.

Response times to Issues, Discussions, and PRs are measured in days and weeks. It is OK to ping people.
Again, this is all my opinion.

[wshanks]

I meant: avoid merging your own PRs. Well, unless it is clear that no one else is going to it.

This is good to have agreement on. When reviewing PRs from other people with write access, my default behavior is to approve and let the author merge, but I can merge more proactively if we want to avoid merging our own PRs here. I think certainly merging your own PR without a review should be avoided.

@wshanks Thanks! I agree with you and @reneeotten that squash-merge here was a fine thing to do immediately after the non-squashed merge, but that this should be viewed as "unusual" and "rarely recommended". And thanks for adding the branch protection.

Agreed!

I also agree with @reneeotten (and from our experience at lmfit/lmfit-py) that squash-merge is sometimes worse than a plain merge. Again, for a mature project with slow-ish development, I think we can make a judgment per PR, and maybe ask to requester to clean up their PR to 2 or 3 commits to hide typos and failed ideas but preserve a bit of "useful" history.

Yes, I agree that having multiple commits is sometimes nicer (that was what I was acknowledging with my comment about rebasing here), but it requires a better understanding of git and more intricate handling of code changes than the squash approach (and can still leave broken commits in the history since usually only the last commit in a PR is tested). We can see what the PRs coming in here look like 🙂 This PR's commit history I don't think added much importance for example. It still gives some but that could be found by seeing the PR number in the squashed commit description and looking up the PR on GitHub.