Skip to content

Conversation

@michaelosthege
Copy link
Member

@michaelosthege michaelosthege commented Mar 18, 2022

Lately we have seen several examples where people asked for permission or commented that they wanted to work on an issue, but did not follow up with a pull request within several weeks or at all.

Because this is blocking other people from getting the work done, and thereby slowing the overall progress, I'm proposing to add a corresponding section to our CONTRIBUTING.md.

The gist of it is:

  • We value getting work done over pledges.
  • We ask people to open Draft PRs as soon as they start working on it.
  • We reserve the right to take over or close PRs when authors go unresponsive.

I think the risk of doing work twice is really low.

Also among high-frequency contributors pledges are rare - if happening at all they are usually more of the kind "I can do this after these two other things are merged".

And in the rare event of two people picking up the same issue at the same time (within 1-3 days) they can help out each other and decide which PR to go for.


Why are the tests triggering? I thought they only run if there are changes in .py files?
(I cancelled them to save resources.)

@ricardoV94
Copy link
Member

ricardoV94 commented Mar 18, 2022

Also, can we remove the guideline to prefix a draft PR with [WIP] (mentioned a couple of lines below your changes)? We should just use the draft tool from Github, it's more clean.

@OriolAbril
Copy link
Member

Why are the tests triggering? I thought they only run if there are changes in .py files?

This https://github.com/pymc-devs/pymc/blob/main/.github/workflows/arviz_compat.yml#L7 paths thing has never worked.

@canyon289
Copy link
Member

Thanks @michaelosthege for creating this, the good news in all this is theres enough interested people that its actually a problem :) Of course one that still should be solved though, I appreciate you opening this PR.

Copy link
Member Author

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

I see we are not on the same page yet regarding the process.

Before we write up a really long list of rules and defintions that few people are going to read, shall we maybe discuss this in the next lab meeting instead?

@michaelosthege michaelosthege force-pushed the contribution-etiquette branch 3 times, most recently from ae77f7d to 009ffea Compare March 18, 2022 17:39
@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #5611 (c4fc278) into main (849a64d) will not change coverage.
The diff coverage is n/a.

❗ Current head c4fc278 differs from pull request most recent head 6afad91. Consider uploading reports for the commit 6afad91 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5611   +/-   ##
=======================================
  Coverage   87.67%   87.67%           
=======================================
  Files          76       76           
  Lines       13717    13717           
=======================================
  Hits        12026    12026           
  Misses       1691     1691           

@michaelosthege
Copy link
Member Author

I just prepended a commit that fixes the paths: filter in our CI pipelines.
The CI runs of this PR are evidence that the change works:

  • The edit of the workflow definitions triggered the workflows
  • The subsequent commits editing the CONTRIBUTING.md only triggered pre-commit and docs build.

Content-wise, I updated larger sections of the guide (it was really outdated!).

If we don't come to an agreement regarding the "I want to work on this" stuff, I would propose to remove that last commit and merge just the CI fix and reformatting for now.

@michaelosthege michaelosthege changed the title Add section on contribution etiquette Fix CI triggers and update contributing guide Mar 18, 2022
@OriolAbril
Copy link
Member

Content-wise, I updated larger sections of the guide (it was really outdated!).

the actual docs might have been kept a bit more to date, but I doubt that. Mid term this page will be gone and be converted to a link to the documentation: #5425

@michaelosthege michaelosthege force-pushed the contribution-etiquette branch 2 times, most recently from 17c7be3 to 164ff54 Compare March 18, 2022 18:30
@michaelosthege
Copy link
Member Author

I rephrased and reformatted the etiquette section again, based on @canyon289's suggestion:

* Any issue without an open pull request is available for work. However if a pull request has no recent activity it may be closed, or taken over by someone else.
* In accordance with the previous point, please don't make unrealistic "_I want to work on this issue_" pledges.

...and also removing some things that were duplicated from the PR steps below.

@canyon289
Copy link
Member

@michaelosthege wanted to say I really appreciate you putting this PR together. Even if I have quetsions about specifics I fully agree some additional guidance is needed on both sides, for contributors and for us so things are smoother.

Thank you prompting this discussion

@michaelosthege michaelosthege force-pushed the contribution-etiquette branch 4 times, most recently from 38e51ed to 1c5a9bd Compare March 19, 2022 09:19
@michaelosthege michaelosthege force-pushed the contribution-etiquette branch from 6e9baf3 to c4fc278 Compare March 19, 2022 09:28
@michaelosthege michaelosthege changed the title Fix CI triggers and update contributing guide Update contributing guide with etiquette section Mar 19, 2022
@ricardoV94
Copy link
Member

Also, can we remove the guideline to prefix a draft PR with [WIP] (mentioned a couple of lines below your changes)? We should just use the draft tool from Github, it's more clean.

Did you manage to do this? I don't see it in the changes

@michaelosthege
Copy link
Member Author

Also, can we remove the guideline to prefix a draft PR with [WIP] (mentioned a couple of lines below your changes)? We should just use the draft tool from Github, it's more clean.

Did you manage to do this? I don't see it in the changes

Yes, it was the old line 95 https://github.com/pymc-devs/pymc/pull/5611/files#diff-eca12c0a30e25b4b46522ebf89465a03ba72a03f540796c979137931d8f92055L95

@michaelosthege michaelosthege force-pushed the contribution-etiquette branch from 0eb1573 to bd82971 Compare March 19, 2022 14:45
@michaelosthege
Copy link
Member Author

michaelosthege commented Mar 19, 2022

After #5619 was just merged, I rebased this branch.
I squashed the 2nd PR template commit into the first and applied @canyon289's suggestion in the other commit.

Looks like the new workflow filters are doing their job. No tests were triggered by these Markdown-only changes :)

EDIT: Just trailing whitespace again 🙄

Co-authored-by: Ravin Kumar <[email protected]>
Co-authored-by: Oriol Abril <[email protected]>
@michaelosthege michaelosthege force-pushed the contribution-etiquette branch from bd82971 to bbe8b98 Compare March 19, 2022 14:54

* All other tests pass when everything is rebuilt from scratch. See
[Developing in Docker](#Developing-in-Docker) for information on running the test suite locally.
* Please select "Create draft pull request" in the dropdown menu when opening your pull request to indicate a work in progress. This is to avoid duplicated work, to get early input on implementation details or API/functionality, or to seek collaborators.
Copy link
Member

@canyon289 canyon289 Mar 19, 2022

Choose a reason for hiding this comment

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

Suggested change
* Please select "Create draft pull request" in the dropdown menu when opening your pull request to indicate a work in progress. This is to avoid duplicated work, to get early input on implementation details or API/functionality, or to seek collaborators.
* Please select "Create draft pull request" in the dropdown menu when opening your pull request to indicate a work in progress. This most importantly will add a claim to the issue ticket if there is one. It also will help avoid duplicated work, to get early input on implementation details or API/functionality, or to seek collaborators.
* In the pull request form in Github paste the link to the issue ticket in the body so Github will link the two together

Copy link
Member

@canyon289 canyon289 left a comment

Choose a reason for hiding this comment

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

Approved, i mostly reworded some things for clarity, and added a couple of extra points

Feel free to reject any of my edits and merge

@michaelosthege michaelosthege merged commit a8e5b53 into main Mar 19, 2022
@michaelosthege michaelosthege deleted the contribution-etiquette branch March 19, 2022 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants