pyo3 icon indicating copy to clipboard operation
pyo3 copied to clipboard

docs: Further updates to Contributing.md re testing, linting etc.

Open MusicalNinjaDad opened this issue 10 months ago • 5 comments

Building on #4080 this further improves the detail and structure of contributing.md regarding testing, linting, CI etc. by separately detailling what is run in PR CI and useful command to run in a dev environment.

~~Furthermore this introduces a quick-CI workflow which runs in approx 3 minutes on all branches except main. This allows contributors to run the most necessary tests locally, offload larger testing efforts to github and still have the confidence that every push will undergo suitable regression testing with quick feedback.~~

MusicalNinjaDad avatar Apr 24 '24 17:04 MusicalNinjaDad

CodSpeed Performance Report

Merging #4115 will not alter performance

Comparing MusicalNinjaDad:contributing (c53208e) with main (059c8b3)

Summary

✅ 67 untouched benchmarks

codspeed-hq[bot] avatar Apr 24 '24 17:04 codspeed-hq[bot]

Thanks, I'm definitely in favor of the docs changes, they look great!

But I'm not so sure how I feel about the CI workflow. (I don't have much experience with GH actions, so if I got something wrong, please correct me.) I assume the workflow is intended to run in forks? Personally I run most of these checks already locally, so for me I would see no need to let them run again in CI on my fork. Most failures I observe originate from feature combinations or Python versions, which are not checked here. I guess it is possible to disable actions on forks that don't want this, but generally I would prefer something like this to be opt-in, rather than opt-out (not sure if that's possible). This would also run on other branches in the pyo3 main repo (I assume), not sure if that's desired.

Thanks @Icxolu, I'm glad you like the doc adjusments.

Yes the idea is that the workflow would run in forks, and yes, it would also run on other branches in the pyo3 main repo.

I take the point of there needing to be an ability to opt-in/out - the simplest way to do this (for both the user and in terms of pipeline complexity) would be based on branch naming: e.g. opt-in by naming your branch ci/somebranch or opt-out by naming it no-ci/somebranch depending on whether the decision is to have the workflow run by default, or not.

Personally I like to offload as much as possible to a pipeline. Given that github actions are effectively free test execution while you can get on with your next adjustments I don't see the disadvantage in having the pipeline run by default - as long as the workflow is kept to a suitable max duration (my pain point is about 200s). It even has the advantage of ensuring all contributors think about fmt, clippy and integration tests as they go, not just when they raise a PR. But I'm open to an opt-in approach if that better suits the views of the contributors here.

FWIW: I also have a very commit-heavy approach compared to most developers, making a very tiny change; running a few quick tests after each change (in rust that's usually just unit tests in the file I just changed), with feedback in a few seconds; then pushing a bundle of 2-5 commits and letting the pipeline handle the rest - I don't mind waiting 2-3 minutes at this point for final feedback. I am particularly lazy and like to have automation look after as much as possible.

MusicalNinjaDad avatar Apr 25 '24 18:04 MusicalNinjaDad

Personally I like to offload as much as possible to a pipeline. Given that github actions are effectively free test execution while you can get on with your next adjustments I don't see the disadvantage in having the pipeline run by default - as long as the workflow is kept to a suitable max duration (my pain point is about 200s). It even has the advantage of ensuring all contributors think about fmt, clippy and integration tests as they go, not just when they raise a PR. But I'm open to an opt-in approach if that better suits the views of the contributors here.

They are a waste of computing resources which run on scarce natural resources if they run unconditionally and redundantly to a local compile-test-edit cycle. I think our current stance here is that the Nox sessions are meant to reproduce as much of the CI locally as is necessary to push changes with reasonable confidence that they pass the pull request CI.

Personally, I would rather invest into making this work than nudging contributors into a centralized GitHub-dependent workflow.

adamreichold avatar Apr 25 '24 18:04 adamreichold

Hi @Icxolu

I have check running in IDE - how do you get clippy to run there, that would be really useful?

I hadn't noticed nox -x. It may be worth also describing which commands it runs and adding it to the list.

I don't think any additional nox options would actually help. I believe I have ADHD and the additional cognitive load of remembering to run those commands is very unpleasant. I hadn't actually realised until this conversation, that having a CI workflow in place was such a valuable asset for me. I knew I relied heavily on them but it was not until I started looking for a viable alternative that I understood why. (Check out the number of commits with the message fmt in #4099 to see the result of "hard things are easy, easy things are hard")

I don't want to be that guy who turns up and in his first couple of contributions reformats the code-base because "my way is better". At the same time I feel emotionally triggered by the expectation to clean up my accomodations as they are bad for the environment and not something the community wishes to promote. I'm OK to conclude that this probably isn't a codebase I will spend much time working on, after all we're doing this for fun so it needs to be a good fit on both sides.

MusicalNinjaDad avatar Apr 29 '24 06:04 MusicalNinjaDad

@MusicalNinjaDad @adamreichold I see where you're both coming from here. Balancing these different needs and goals seems difficult, but I'd like to find a way forward that allows @MusicalNinjaDad to continue to contribute and feel welcome.

LilyFoote avatar Apr 29 '24 10:04 LilyFoote