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

RFC Governance Document #12878

Merged
merged 12 commits into from Feb 25, 2019
Merged

RFC Governance Document #12878

merged 12 commits into from Feb 25, 2019

Conversation

@amueller
Copy link
Member

@amueller amueller commented Dec 27, 2018

This is a draft of the proposed governance structure for the scikit-learn project. It has been discussed somewhat between core developers, but I want to get input from the wider community.
We tried to keep is as simple as possible though some complexities crept in as always.

I did some editing to have a consistent document, so this is "my version", not a consensus (yet).

The goal is to discuss this document further in this PR and have a final vote of core developers on the public mailing list.

I kicked out the (really old and bad) flow chart from the documentation page (it's still in the tutorial). Alternatively we could remove the "other resources" which are pretty out of date and google probably provides better ones.

initial rendering:
https://41695-843222-gh.circle-artifacts.com/0/doc/governance.html

@@ -87,13 +87,6 @@ Documentation of scikit-learn |version|
</div>

<div class="row-fluid">
<div class="span4 box">
<h2><a href="tutorial/machine_learning_map/index.html">Flow Chart</a></h2>

This comment has been minimized.

@qinhanmin2014

qinhanmin2014 Dec 28, 2018
Member

Maybe remove Additional Resources instead? The flow chart seems useful from my side and most additional resources are outdated.

This comment has been minimized.

@jnothman

jnothman Jan 10, 2019
Member

I think it would be fine to see governance linked from About and Developers and not have this prominent a position

This comment has been minimized.

@amueller

amueller Jan 15, 2019
Author Member

@jnothman so you don't want to see the additional resources go?

This comment has been minimized.

@jnothman

jnothman Feb 11, 2019
Member

I just don't see the value in having this as a core section of the Documentation page. Most users don't care. Most developers don't care. It is not, in a sense, about the software, but about about the software (so it belongs on About and Developers).

doc/governance.rst Outdated Show resolved Hide resolved
doc/governance.rst Outdated Show resolved Hide resolved
doc/governance.rst Outdated Show resolved Hide resolved
doc/governance.rst Outdated Show resolved Hide resolved
Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

LGTM, personally I'd like to keep the flow chart.

@reshamas
Copy link
Collaborator

@reshamas reshamas commented Dec 30, 2018

Question for you:
For example, if a contributor does not respond to a review comment in PR, when can someone else take it over? What's the etiquette for that? Do they wait 1 week, 1 month? Or follow-up 1-3 times before taking it over?
(refer to wimlds scikit-sprint for examples)

doc/governance.rst Outdated Show resolved Hide resolved
comments) in the past 12 months will be asked if they want to become emeritus
core developers and recant their commit and voting rights until they become
active again. The list of core developers, active and emeritus (with dates at
which they became active) is public on the scikit-learn website.

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Dec 30, 2018
Member

Ideally we should add a link here to this list. However, I would suggest that this is done in a second time, after the merge of this document.

This comment has been minimized.

@amueller

amueller Dec 30, 2018
Author Member

I added a link to the group which is the active members, but yes, once we have a list we can link it here.

This comment has been minimized.

@satra

satra Feb 11, 2019
Member

as someone who falls in this category, i think this is a really good idea.

This comment has been minimized.

@jnothman

jnothman Feb 11, 2019
Member

I'm not, tbh, sure what benefit we have of putting dates on the dev list is...

This comment has been minimized.

@amueller

amueller Feb 12, 2019
Author Member

I'm not super sold on it but we can try? ;)

This comment has been minimized.

@amueller

amueller Feb 12, 2019
Author Member

I think the main motivation is that we show how long people were active to give proper credit?

This comment has been minimized.

@jnothman

jnothman Feb 12, 2019
Member

If we're trying to "show how long", then "dates at which they became active" is not really sufficient!

This comment has been minimized.

@amueller

amueller Feb 19, 2019
Author Member

If we have dates of when they became active and when inactive that shows how long, right?

doc/governance.rst Outdated Show resolved Hide resolved
@GaelVaroquaux
Copy link
Member

@GaelVaroquaux GaelVaroquaux commented Dec 30, 2018

@reshamas :

For example, if a contributor does not respond to a review comment in PR, when can someone else take it over? What's the etiquette for that? Do they wait 1 week, 1 month? Or follow-up 1-3 times before taking it over?

This is a good question, and I feel that we should address it. My personal preference would be:

  1. new contributor comments on PR saying that she/he wants to take over
  2. failing to respond to respond to respond to comments for 1 week means PR can be taken over
  3. a comment simply saying that the initial contributor wants to continue is valid, however such comment should give a timeframe

However, first this is only a personal view, second I feel that this should go in the contributor guide, rather than in the governance document.

(refer to wimlds scikit-sprint for examples)

Would you mind giving a link: I feel that there is good material that can be reused.

@reshamas
Copy link
Collaborator

@reshamas reshamas commented Dec 30, 2018

@GaelVaroquaux

  1. Here is the sprint list of PRs
  2. Here are a couple of examples:

Side note: I also reached out to these contributors multiple times via email and Meetup email, and those communications are not documented on GitHub.

@GaelVaroquaux
Copy link
Member

@GaelVaroquaux GaelVaroquaux commented Dec 30, 2018

@reshamas : I am starting to understand (sorry, I have not been involved with the sprint, although it would have been a good use of my time): you, and others, lost time and were not able to contribute productively because of these stalled PRs.

As far as I am concerned, we should indeed move forward on such stalled PR. The only caveat is that sometimes a contributor has in his agenda to finish, but is not checking email at a specific time. I really think that your concern is a legitimate one, and we should address it by making things more explicit. When the project was small, the community made decisions on a case-by-case basis. With a much bigger community, it doesn't work well. I propose write guidelines in a separate pull request, and leave this one for the main governance questions.

@amueller
Copy link
Member Author

@amueller amueller commented Dec 30, 2018

@reshamas I agree this is an important issue, but I would also argue this should probably not part of the governance doc.
@GaelVaroquaux I was helping @reshamas with the sprint and I think overall it went pretty well :)

@amueller
Copy link
Member Author

@amueller amueller commented Dec 30, 2018

Thank you for all your input @GaelVaroquaux! I'll try to respond before my vacation starts on Tuesday but I have a long list of things to do.

fix broken link

Co-Authored-By: amueller <t3kcit@gmail.com>
@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Dec 31, 2018

So we've decided to drop the great flow chart? I still think it's better to remove Additional Resources instead.

@amueller
Copy link
Member Author

@amueller amueller commented Jan 9, 2019

@qinhanmin2014 I wouldn't say that we have decided that. I think no-one else has voiced their opinion (looks like @TomDLT agrees with you). I'm happy to make the change but this is not the part I'm most concerned about ;)

There's been no movement on this for 10 days. I think I had actually commented on the issue brought up by @GaelVaroquaux and I like @adrinjalali's suggestion.

I'm a bit disappointed by the lack of engagement I have to say but I guess that's what we get for a lengthy process. Given the lack of input, I think we should go with @adrinjalali's suggestion, give everything another reading, and then probably put it to a vote on the general mailing list, with all "core devs" having the ability to vote?

@@ -87,13 +87,6 @@ Documentation of scikit-learn |version|
</div>

<div class="row-fluid">
<div class="span4 box">
<h2><a href="tutorial/machine_learning_map/index.html">Flow Chart</a></h2>

This comment has been minimized.

@jnothman

jnothman Jan 10, 2019
Member

I think it would be fine to see governance linked from About and Developers and not have this prominent a position

doc/governance.rst Outdated Show resolved Hide resolved
doc/governance.rst Outdated Show resolved Hide resolved

Scikit-learn uses a "consensus seeking" process for making decisions. The group
tries to find a resolution that has no open objections among core developers
within a month. If no consensus can be reached in this time frame, a

This comment has been minimized.

@jnothman

jnothman Jan 10, 2019
Member

within a month of what? How/when does one declare that a decision needs to be made? Does one create a SLEP to force a decision, such that then there is a month from its posting to find consensus? Can this be made more explicit?

This comment has been minimized.

@adrinjalali

adrinjalali Jan 10, 2019
Member

When I tried to fix this issue on the doc version, I failed to keep it simple, but it was along the lines of: "A deadline is set once two core developers call for a deadline on a particular issue."

This comment has been minimized.

@amueller

amueller Jan 15, 2019
Author Member

This is indeed a tricky point and I'm not sure how to best address it.
Go has (shepherds)[https://github.com/golang/proposal] that can call for votes if I understand correctly. I'm not sure how well that works.
Having a call for vote and a seconding would also work. Can the proposer call for a vote and/or second?

This comment has been minimized.

@jnothman

jnothman Jan 16, 2019
Member

I'm not sure what you're asking in

Can the proposer call for a vote and/or second?

I'm fine with two people proposing a vote, but it seems absent here.

This comment has been minimized.

@adrinjalali

adrinjalali Jan 16, 2019
Member

... The group
tries to find a resolution that has no open objections among core developers
within a month. ...

->

... The group
tries to find a resolution that has no open objections among core developers.
At any given time, if a core developer suggests a deadline for the discussion
and another core developer seconds the suggestion, that deadline is the time
the team has to reach a consensus. ...

This comment has been minimized.

@jnothman

jnothman Jan 16, 2019
Member

but then you need to stipulate a minimum time to deadline

This comment has been minimized.

@adrinjalali

adrinjalali Jan 16, 2019
Member

My first draft had "a deadline which is not closer that two weeks from the point of setting the deadline", but it becomes a bit convoluted in the text. I just thought if we require two people to sign on the deadline, they'd agree on a "reasonable" deadline. I'm personally happy with having a min and/or max on the deadline call.

This comment has been minimized.

@amueller

amueller Jan 17, 2019
Author Member

@jnothman The question was whether to do a vote, we need two people that are not the proposer to call for a vote, or if the proposer can be one of the two people.

I'll try to reformulate this in a bit.

This comment has been minimized.

@NicolasHug

NicolasHug Jan 28, 2019
Member

within a month of what? How/when does one declare that a decision needs to be made? Does one create a SLEP to force a decision, such that then there is a month from its posting to find consensus?

According to this document the SLEPs are explicitly used to call for a vote. So that seems like a natural solution to me. But it doesn't have to be a strict requirement, just a possibility.

How about:

  • call for a vote = vote will take place in 1 month.
  • any call for a vote must be backed by a SLEP
  • any core dev can call for a vote, regardless of who created the SLEP
  • SLEP creation != call for a vote, though the proposer can do both at the same time
adrinjalali and others added 4 commits Jan 15, 2019
Co-Authored-By: amueller <t3kcit@gmail.com>
@amueller
Copy link
Member Author

@amueller amueller commented Jan 29, 2019

Ok I tried to simplify taking the process by adding a call to vote. So now any core dev can call a vote on any SLEP and the vote takes a month. If no 2/3 majority is reached, it goes to the TC which has another month.
I think this is ample time to make a decision. I'd really like to move forward with this and ideally approve and merge before the sprint.

Sorry for my delay on finalizing this.
If there's no major comments in the next week I'll call for a vote on the general mailing list.


Scikit-learn uses a "consensus seeking" process for making decisions. The group
tries to find a resolution that has no open objections among core developers.
At any point during the discussion, any core-developer can call for a vote, which will

This comment has been minimized.

@adrinjalali

adrinjalali Jan 31, 2019
Member

I still think it makes sense to include the requirement of another core developer to second the call for the vote though. But no hard feelings.

This comment has been minimized.

@amueller

amueller Feb 2, 2019
Author Member

I feel like that would complicate things and we can always change this if it doesn't work out. What do others think?
I'm not really opposed to it but I feel like we should try and keep things as simple as possible.

This comment has been minimized.

@jnothman

jnothman Feb 4, 2019
Member

I think in practice a single call for vote should be okay.

This comment has been minimized.

@amueller

amueller Feb 6, 2019
Author Member

Ok. I'll wait a couple more days for people to comment.

@amueller
Copy link
Member Author

@amueller amueller commented Feb 6, 2019

I'll call for a vote on the ML on Friday, then we can have two weeks of voting (possibly longer if we don't get a good turnout?) and can merge before we (that is non paris people) head out to the sprint.

I want the sprint to be spend bikeshedding the sleps, not the governance ;)

@NelleV
NelleV approved these changes Feb 9, 2019
Copy link
Member

@NelleV NelleV left a comment

👍


This is a meritocratic, consensus-based community project. Anyone with an
interest in the project can join the community, contribute to the project
design and participate in the decision making process. This document describes

This comment has been minimized.

@NelleV

NelleV Feb 9, 2019
Member

minor nitpick: there should probably be a comma after the word "design"

This comment has been minimized.

@jnothman

jnothman Feb 11, 2019
Member

the Oxford comma...

consensus in the required time frame, the TC is the entity to resolve the
issue.
Membership of the TC is by nomination by a core developer. A nomination will
result in discussion which cannot take more than a month and then a vote by

This comment has been minimized.

@NelleV

NelleV Feb 9, 2019
Member

similarly, a comma is missing after "month"

decision is escalated to the TC, which in turn will use consensus seeking with
the fallback option of a simple majority vote if no consensus can be found
within a month. This is what we hereafter may refer to as “the decision making
process”.

This comment has been minimized.

@NelleV

NelleV Feb 9, 2019
Member

nitpick: process''. -> process.''

This comment has been minimized.

@jnothman

jnothman Feb 11, 2019
Member

I always understood this as being UK-US variation.

tries to find a resolution that has no open objections among core developers.
At any point during the discussion, any core-developer can call for a vote, which will
conclude one month from the call for the vote. Any vote must be backed by a
`SLEP <slep>`. If no option can gather two thirds of the votes cast, the

This comment has been minimized.

@NelleV

NelleV Feb 9, 2019
Member

This phrasing is a bit ambiguous: are options referring to "yes" or "no?" Or is it referring to several technical options, and core developers don't agree with the same technical option?

Maybe I'm being to nitpicky here: I don't know how often this situation happens. We can always revisit if the TC is swamped with voting requests.

This comment has been minimized.

@amueller

amueller Feb 11, 2019
Author Member

Yes, it's ambiguous, I agree. I think right now it allows both (yes/no or different options). For things like freezing or pipeline slicing it might be less a question of whether we want it but how. We could have one SLEP and vote per technical option, I'm not sure how this would play out in practice. I think we can figure this out as we go along.

will be unanimous, a two-thirds majority of the cast votes is enough. The vote
needs to be open for at least 1 week.

Core developers that have not contributed to the project (commits or GitHub

This comment has been minimized.

@NelleV

NelleV Feb 9, 2019
Member

As a side note, this is a bit in contradiction with "not only code contributions are valued." This doesn't include any organization of sprints, fundraising, etc.

This comment has been minimized.

@amueller

amueller Feb 11, 2019
Author Member

I think we can change that to "having actively contributed (commits, comments or organizational and non-code contributions)". I'm slightly hesitant about changing things now, but I think it would be consistent with the spirit of the document to rephrase this.

This comment has been minimized.

@jnothman

jnothman Feb 11, 2019
Member

Recall, though, that the emeritus status is only negotiated ("will be asked if they want to become emeritus"), triggered by this criterion. Explicitly using a criterion of commits or comments makes it much easier to measure activity by which this emeritus proposition is triggered. A vague criterion is not especially helpful here. There is nowhere that says the emeritus status will be forced, though presumably this would follow the same as any other personnel dispute (not explicitly covered in this document), such as removing core dev rights.

This comment has been minimized.

@amueller

amueller Feb 12, 2019
Author Member

I think the suggestion was more in terms of what he document communicates. I'm not sure we'll use a very precise criterion anyway (unless you wanted to write a bot ;). I figured it would be more that either we'll do a (yearly?) spring cleaning or someone thinks "has been awhile since this person was around"?
I don't have strong feelings either way, though.

@GaelVaroquaux
Copy link
Member

@GaelVaroquaux GaelVaroquaux commented Feb 9, 2019

@amueller
Copy link
Member Author

@amueller amueller commented Feb 11, 2019

Thanks for the approves. I would prefer if everybody voted on the mailing list as this is the mechanism specified in the document, and also more public and seems easier to get a record of @TomDLT @NelleV @agramfort.

@GaelVaroquaux
Copy link
Member

@GaelVaroquaux GaelVaroquaux commented Feb 11, 2019

@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Feb 12, 2019

I just don't see the value in having this as a core section of the Documentation page. Most users don't care. Most developers don't care. It is not, in a sense, about the software, but about about the software (so it belongs on About and Developers).

Fair enough, should we remove doc/tutorial/machine_learning_map/ then? @jnothman

@jnothman
Copy link
Member

@jnothman jnothman commented Feb 12, 2019

Fair enough, should we remove doc/tutorial/machine_learning_map/ then? @jnothman

I'm trying to say that this PR should not be touching doc/documentation.rst so that question seems irrelevant.

@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Feb 12, 2019

I'm trying to say that this PR should not be touching doc/documentation.rst so that question seems irrelevant.

Sorry I misunderstood your comment. I don't have a strong opinion about where to put the governance document, just feel that the road map is useful from my side.

rephrasing

Co-Authored-By: amueller <t3kcit@gmail.com>
@amueller
Copy link
Member Author

@amueller amueller commented Feb 19, 2019

@jnothman I'm fine with moving it to the about page.

@amueller
Copy link
Member Author

@amueller amueller commented Feb 19, 2019

It would be great if y'all could vote or abstain on the mailing list
@adrinjalali @agramfort @lesteve @yarikoptic @arjoly @glouppe @fabianp @kastnerkyle @MechCoder @jmetzen @jakevdp @bthirion.

Thanks!

Copy link
Member

@jnothman jnothman left a comment

We should link to this from the contributing docs too. But this lgtm

Copy link
Member

@MechCoder MechCoder left a comment

looks great, thanks!

@agramfort
Copy link
Member

@agramfort agramfort commented Feb 20, 2019

@amueller I already upvoted on Feb 10 on the list

@satra
Copy link
Member

@satra satra commented Feb 20, 2019

@amueller - did you want us to specifically respond on the list, or does an upvote on the original post sufficient (which i did).

@amueller
Copy link
Member Author

@amueller amueller commented Feb 20, 2019

@agramfort oh sorry, my bad :-/ somehow the thread got split and I didn't take the first part into account, also had Gilles and Adrin.

@amueller
Copy link
Member Author

@amueller amueller commented Feb 20, 2019

@satra given the phrasing of the document for decision making I think it would be good if you posted on the list if you don't mind.

@amueller
Copy link
Member Author

@amueller amueller commented Feb 22, 2019

I didn't reach out to the other devs as I originally planned for time reason, but we have 25 votes for, zero against or explicit abstain, and 24 non-responses. I think we're good to merge. I'm not sure how we should record the outcome, is the mailing list archive enough?

@GaelVaroquaux
Copy link
Member

@GaelVaroquaux GaelVaroquaux commented Feb 22, 2019

@jmetzen
Copy link
Member

@jmetzen jmetzen commented Feb 23, 2019

looks good to me, thanks team!

@amueller amueller merged commit 8a258c9 into scikit-learn:master Feb 25, 2019
11 checks passed
11 checks passed
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
@codecov
codecov/patch Coverage not affected when comparing 35fcd86...91561e1
Details
@codecov
codecov/project 92.47% remains the same compared to 35fcd86
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GaelVaroquaux
Copy link
Member

@GaelVaroquaux GaelVaroquaux commented Feb 25, 2019

jnothman added a commit to jnothman/scikit-learn that referenced this pull request Feb 26, 2019
* add governance doc

* add links to docs

* minor formatting, more links

* Update doc/governance.rst

fix broken link

Co-Authored-By: amueller <t3kcit@gmail.com>

* Apply suggestions from code review

Co-Authored-By: amueller <t3kcit@gmail.com>

* reframed with call for vote. I think this is good.

* make it easier to call a vote.

* Update doc/governance.rst

rephrasing

Co-Authored-By: amueller <t3kcit@gmail.com>

* link governance from the about page instead on documentation.rst
xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019
* add governance doc

* add links to docs

* minor formatting, more links

* Update doc/governance.rst

fix broken link

Co-Authored-By: amueller <t3kcit@gmail.com>

* Apply suggestions from code review

Co-Authored-By: amueller <t3kcit@gmail.com>

* reframed with call for vote. I think this is good.

* make it easier to call a vote.

* Update doc/governance.rst

rephrasing

Co-Authored-By: amueller <t3kcit@gmail.com>

* link governance from the about page instead on documentation.rst
xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019
xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde added a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
* add governance doc

* add links to docs

* minor formatting, more links

* Update doc/governance.rst

fix broken link

Co-Authored-By: amueller <t3kcit@gmail.com>

* Apply suggestions from code review

Co-Authored-By: amueller <t3kcit@gmail.com>

* reframed with call for vote. I think this is good.

* make it easier to call a vote.

* Update doc/governance.rst

rephrasing

Co-Authored-By: amueller <t3kcit@gmail.com>

* link governance from the about page instead on documentation.rst
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet