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

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MusicalNinjaDad
Copy link

@MusicalNinjaDad MusicalNinjaDad commented Apr 24, 2024

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.

Copy link

codspeed-hq bot commented Apr 24, 2024

CodSpeed Performance Report

Merging #4115 will not alter performance

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

Summary

✅ 69 untouched benchmarks

Copy link
Contributor

@LilyFoote LilyFoote left a comment

Choose a reason for hiding this comment

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

This looks really useful! Thanks!

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

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.

@MusicalNinjaDad
Copy link
Author

MusicalNinjaDad commented Apr 25, 2024

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.

@adamreichold
Copy link
Member

adamreichold commented Apr 25, 2024

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.

Contributing.md Outdated Show resolved Hide resolved
@MusicalNinjaDad MusicalNinjaDad changed the title docs: Further updates to Contributing.md re testing, linting etc. and a quick-CI github workflow to make contributor's lives even easier. docs: Further updates to Contributing.md re testing, linting etc. and a quick-CI github workflow to make contributors' lives even easier. Apr 26, 2024
@MusicalNinjaDad MusicalNinjaDad changed the title docs: Further updates to Contributing.md re testing, linting etc. and a quick-CI github workflow to make contributors' lives even easier. docs: Further updates to Contributing.md re testing, linting etc. Apr 27, 2024
@MusicalNinjaDad
Copy link
Author

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.

@LilyFoote
Copy link
Contributor

@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.

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.

None yet

4 participants