slep000, slep workflow #30
Conversation
|
I think this also supersedes #28. |
There was a problem hiding this comment.
LGTM on the document. I just have a reserve during the process.
Once a SLEP is merged and we asked to the mailing list to vote by linking to a PR changing the status to "Accepted", we might still need to have this SLEP opened to comment in the PR.
What I mean is that a this stage only the line which has been changed will be in the diff (meaning the Status). Thus, we will not be to specifically discuss a detail related to a section. I think that we should be able to do so.
slep000/proposal.rst
Outdated
|
|
||
| Once the PR for the SLEP is in place, a post should be made to the mailing list | ||
| containing the sections up to “Backward compatibility”, with the purpose of | ||
| limiting discussion there to usage and impact. Discussion on the pull request |
There was a problem hiding this comment.
Are you suggesting to also enable discussion on the mailing list? Wouldn't this scatter/fraction the discussion?
There was a problem hiding this comment.
Yes it would, but some of the discussions where mostly non-core-devs are involved and give feedback, doesn't have to be on the PR.
There was a problem hiding this comment.
Given that this is also the NEP process that seems fine, I think?
slep000/proposal.rst
Outdated
| limiting discussion there to usage and impact. Discussion on the pull request | ||
| will have a broader scope, also including details of implementation. | ||
|
|
||
| At the earliest convenience, the PR should be merged (regardless of whether it |
There was a problem hiding this comment.
what is "earliest convenience"?
Should -> may (let's at least not make it a mandatory process please)
There was a problem hiding this comment.
This is the main reason I opened this SLEP. If there's one thing I would argue for, is this one.
There was a problem hiding this comment.
Then what does "At the earliest convenience" mean?
Depending on the interpretation, that would mean you could just merge this PR right now. I doubt this is a productive workflow.
Also imagine a situation where I'm writing a SLEP PR, and you and I disagree on some point. I'm merging the PR, because I have to, according to this SLEP. Your points in our discussion are "lost" now. You have to make them again, in another PR. That's twice as much work for you.
There was a problem hiding this comment.
I would argue that right now is a really good point to merge it actually. And that applies to the feature names out, and the resampler one. They may need more work, but the whole point is that they should be merged now.
We can have issues and separate PRs discussing specific points, for instance, one for alternatives, one for abstract, etc.
There was a problem hiding this comment.
Can you explain why you think it would ease the SLEP process to merge early and discuss in separate PRs?
In the end, comments will be made, and they will have to be addressed. I'm failing to see how having separate PRs will help. I understand the initial frustration was that on long PRs, it's hard to keep track of the changes and the discussions. But that doesn't seem to apply to all SLEPs, so I don't see why we should make this mandatory
There was a problem hiding this comment.
For the same reason that we break large PRs into several smaller ones in many cases
This is where I disagree: we have merged PRs with reduced scope, but as far as I know, we do not merge PRs that aren't properly finished yet, with the hope that issues will be addressed later.
Typically, I consider scikit-learn/scikit-learn#16112 to be one of these PRs with a reduced scope: it's addressing one part of SLEP010. But I'm making sure it doesn't get merged until all the points that are raised (and that are in scope) are addressed.
There was a problem hiding this comment.
both python and numpy communities agree that when it comes to *EPs, they should get merged before they are properly finished, and I agree with them.
There was a problem hiding this comment.
I still don't believe this should be an obligation. But we don't have to agree
There was a problem hiding this comment.
So the NEP says " Additional PRs may be made by the Author to update or expand the NEP, or by maintainers to set its status, discussion URL, etc.", meaning that the SLEP author is the only one who is supposed to change it, right?
I think it's fine to merge drafts, and it's up to the author to ensure the draft is mature enough to serve as a basis for discussion.
I'm not entirely sure how the process for NEPs and PEPs works in terms of the back and forth between the author and the community. I feel all the important points should be part of the SLEP so losing the detailed discussion on a PR might be ok?
Maybe we can ping the numpy people to tell us about their experience?
If I look at " Discussion on the pull request will have a broader scope, also including details of implementation." it sounds like the main discussion does happen on the pull request.
There was a problem hiding this comment.
To comment on this point: I feel that my view sits a bit in the middle of that of @NicolasHug and of @adrinjalali :
- I really see the value of merging things that are consolidated but there still is open discussion
- We need them to be "consolidated" (we could punt on defining precisely what that means).
How about we say that they can be merged when the author considers them as ready for discussion and at least on core-dev is willing to champion this merge (that last statement is to avoid polluting the SLEPs repo with SLEPs that have no chances of making it).
|
I sort of have the same comment as Joel on #28 (review), i.e. this seems to be addressing non-issues, for the most part. The main points seem to be:
We can probably make a much shorter SLEP that addresses these 2? |
I think people could talk about the rest of it still, and if changes are made, then it'd be on the diff. But this is due to the fact that other communities have those discussions on the mailing list, and we're having it on the PR. Numpy for instance, just says "hey, we're accepting this, unless somebody seriously objects in 7 days". We may be having too many discussions compared to other communities :P |
It may be a non-issue for some, but it is for me, and I understand from my conversations with @glemaitre it is for him as well.
you can try, this is the best/shortest I could do. |
|
I have two main concerns about the current proposal.
The common denominator to both points here is that our bottleneck is reviewer time, not developer time. I fear that this proposal will put more responsibility on reviewers than necessary. I have the feeling that the discussion so far has been rather defensive (from all sides), here and in other PRs. I'll try to be more vigilant in my comments so they don't get interpreted as such. I do share the frustration around writing and reviewing SLEPs. I believe however that the key to a smoother process is timely feedback, from both reviewer and author sides. If this opinion is shared, I'd be happy to open discussion for potential workflows, e.g. in the next meeting. |
There was a problem hiding this comment.
Overall, I really like this proposal.
Thinking about it, I think that the underlying idea of @adrinjalali of merging early is a good one. Very long pull requests are difficult to follow. The knowledge acquired from them can also be lost if they stall.
To summarize my thoughts on this SLEP (somewhat present in my comments):
-
I think that a SLEP should be merged, in draft mode, when the author considers them as ready for discussion and at least on core-dev is willing to champion this merge
-
I think that discussions happening on the SLEP (whether it is on the mailing list or on a PR) should be consolidated in the SLEP as much as possible. "As much as possible" means that the text of the SLEP should be kept to a reasonable length: an expected reading speed is 200 words per minute, ideally the body of a SLEP should not take longer than 10mn to read.
-
A mentioned in my comments, I would see it useful to have mechanisms to reject a SLEP not only when it's champion gives up of exhaustion.
@adrinjalali : do you want to give my point 2 a try on this specific SLEP? To see if it is viable or not?
Thanks for doing this!
|
|
||
| If the vote does not achieve a required majority, then the SLEP remains in | ||
| ``Draft`` state, discussion continues as normal, and it can be proposed for | ||
| acceptance again later once the objections are resolved. |
There was a problem hiding this comment.
Do we need a mechanism for rejecting clearly a SLEP?
The reason that I ask this is that I can be useful to avoid people loosing a lot of time on SLEP that is considered as not having chances of getting through?
Maybe the "rejection" could and should be done via a pull request to the SLEP that explains the reason for rejecting it?
There was a problem hiding this comment.
The SLEP in its current state says:
The
Withdrawnstatus is similar; it means that the SLEP author themselves has
decided that the SLEP is actually a bad idea, or has accepted that a competing
proposal is a better alternative.
I think it's important for the SLEP authors to make that decision if they wish, and otherwise leave it to the vote. rejected and withdrawn states are effectively the same. WDYT?
There was a problem hiding this comment.
In other words, if not by the author's decision (not necessarily exhaustion) and not the vote, how else would you like to be able to reject a SLEP?
There was a problem hiding this comment.
I agree with @adrinjalali I think the mechanisms described here should be enough.
There was a problem hiding this comment.
In the case where the original author does not feel like withdrawing a SLEP after a failed vote and the majority things that it should be withdrawn or rejected, what should we do?
Is it acceptable for someone else to do a PR to reject the slep and then use the usual decision making described in the governance doc to deal with the rejection PR?
If so maybe we should make it a bit more explicit here. Maybe just amending the next section to:
In unusual cases, with the request of the author, the scikit-learn technical
committee may be asked to decide whether a controversial SLEP is ``Accepted``, put back to ``Draft`` with additional recommendation to try again to reach consensus or definitely ``Rejected``. See the governance doc for more details.
There was a problem hiding this comment.
In the case where the original author does not feel like withdrawing a SLEP after a failed vote and the majority things that it should be withdrawn or rejected, what should we do?
Is it acceptable for someone else to do a PR to reject the slep and then use the usual decision making described in the governance doc to deal with the rejection PR?
If so maybe we should make it a bit more explicit here. Maybe just amending the next section to:
In unusual cases, with the request of the author, the scikit-learn technical
committee may be asked to decide whether a controversial SLEP is ``Accepted``, put back to ``Draft`` with additional recommendation to try again to reach consensus or definitely ``Rejected``.
Please refer to the governance doc for more details.
There was a problem hiding this comment.
hm so the ambiguity on how to reach the "rejected" state is copied from the NEP. The PEP doesn't have that problem because they basically delegate the decision to an individual that then makes the call and can reject, or the steering council can reject. With the NEP version that goes back to draft there's no way to get rejected. Anyway I think it's fine the way it is for now, we can always clarify later.
I agree. It's just hard to put it in words in a short way, and I think that's why NEP000 has it as such (which this SLEP borrows). PEP1 has a much clearer (and yet similar IMO) process:
Agreed. In my mind an issue related to a SLEP is only resolved if the resolution is included in the SLEP. So that would kinda automatically be included. We can include it in the SLEP if you think it'd help/is necessary. |
|
looks good. I like the PEP wording and maybe would include a shortened version of it. I'm not sure if we should include this here, but for normal PRs it's very rare that someone merges their own PR except for in trivial cases. Should we do the same for SLEPs? |
|
I tried to revise the merge criteria, let me know how it looks. |
slep000/proposal.rst
Outdated
| criteria are: | ||
|
|
||
| - It sound and complete. The ideas must make technical sense. | ||
| - The reviewers do not consider whether they seem likely to be accepted. |
There was a problem hiding this comment.
I don't understand this: this doesn't seem to be a criteria?
There was a problem hiding this comment.
I'm not sure if I understand the question. Maybe going through PEP1 would help?
slep000/proposal.rst
Outdated
| The SLEP is reviewed for structure, formatting, and other errors. Approval | ||
| criteria are: | ||
|
|
||
| - It sound and complete. The ideas must make technical sense. | ||
| - The reviewers do not consider whether they seem likely to be accepted. | ||
| - The title accurately describes the content. | ||
|
|
||
| Reviewers are generally quite lenient about this initial review, expecting that | ||
| problems will be corrected by the further reviewing process. **Note**: Approval | ||
| of the SLEP is no guarantee that there are no embarrassing mistakes! Ideally | ||
| they're avoided, but they can also be fixed later in separate PRs. Once | ||
| approved by at least one core developer, the SLEP can be merged. Additional PRs | ||
| may be made by the champions to update or expand the SLEP, or by maintainers to | ||
| set its status, discussion URL, etc. |
There was a problem hiding this comment.
It's not clear to me whether this section refers to approval of the first PR, or actual approval of the SLEP. I assume it's the former, but the text often suggests the latter.
There was a problem hiding this comment.
This part is about approval of the PR.
There was a problem hiding this comment.
Therefore I would kindly suggest to edit the parts were the text suggest that this is about SLEP approval:
-
The SLEP is reviewed for structure, formatting, and other errors. Approval
criteria are... -
Approval of the SLEP...
-
Once approved by at least one core developer, the SLEP...
slep000/proposal.rst
Outdated
| problems will be corrected by the further reviewing process. **Note**: Approval | ||
| of the SLEP is no guarantee that there are no embarrassing mistakes! Ideally | ||
| they're avoided, but they can also be fixed later in separate PRs. Once | ||
| approved by at least one core developer, the SLEP can be merged. Additional PRs | ||
| may be made by the champions to update or expand the SLEP, or by maintainers to | ||
| set its status, discussion URL, etc. |
There was a problem hiding this comment.
This section still mentions "SLEP approval" instead of "PR approval".
|
I pretty much opened this PR to make sure we merge the SLEP PRs as fast as we can. I had terrible experience working on a few SLEPs as the author or a co-authors which motivated me to check the PEP and NEP processes. Both of those processes merge the PR before it's ready for acceptance, and further discussion is supposed to happen on other channels, such as a forum, mailing list, or github issues. I understand you don't want PRs to be merged before a consensus is there and all points are addressed, but that's exactly what's made me not work on SLEPs as I was before. Also, I don't understand why this process would work for python and numpy, but not scikit-learn. |
Same. I very much share your frustration here. I have a full proper SLEP that took me days to write, and that I never submitted, because of the pain from previous experiences. I also had a terrible experience as the reviewer with this new proposed workflow. That reviewer experience just plainly made me stop even caring about SLEP006 (and to be fair, about most other SLEPs as well). |
I can create a script to automate this migration. It will open the new PR and create the review comments with quotes and links to unresolved discussion points from the previous PR. In the end, I want to make the SLEP process more enjoyable for everyone. |
|
Maybe we could talk to the NEP / SLEP folks to see how they structure the discussion afterwards? I think it has mostly been on mailing lists, but when I was discussing SLEP 001 here, not sure anyone ever looked, I found the lack of inline citation annoying as well. I think the main question is, if we merge early, how do we get from there to a place where we can vote on. |
@NicolasHug That's sad. I'm sure we would profit from your thoughts if you publish them, one way or the other. We are misusing github's code development tools for design documents. I'm wondering if, in the end, it's a question about tools. For instance, I like the early merge logic simply because I like to read rendered text instead of source code. Maybe someone can tell from experience? I hope we find a good solution we all can agree upon as SLEPs are important to bring innovation/improvements on a larger scale. |
|
Closing and reopening PRs, along with a rendering (or better something like reviewnb.com for Sphinx docs?) sounds like a reasonable solution to some of the issues here. But maybe the point is that we need to clarify the problems rather than using SLEP000 as a bandaid. |
|
I too prefer having the rendered content of the SLEP when reviewing, but I just build this repo's docs locally, it only takes a few seconds I wouldn't mind having a dedicated section on the website for in-review SLEPs, as long as the open PR still contains the entire content of the SLEP. |
|
I agree with both concerns and experience exposed by @adrinjalali and @NicolasHug and I personally do not have a process in mind.
I am wondering if @lorentzenchr does not have point there and if a Google Doc with the capacity of having the rendered version, proposing some editing, adding some comments would not be a better tool. I can see that it miss the code part (but some extensions should be available there). |
At least 3 core devs have expressed concerns about the current poposal:
There also has been quite a few suggestions of alternative workflows for moving forward, e.g.:
I'm really sorry but until these are addressed in some way, I don't think this SLEP is mature enough to be voted upon. |
|
I'm really sorry but until these are addressed in some way, I don't think this SLEP is mature enough to be voted upon.
The feeling at the call yesterday was in favor of voting.
|
|
I wish I could have attended the meeting. Were any of the aforementioned concerns discussed? I could not find anything specific in the meeting notes. |
|
Maybe, only maybe, we should dare calling for votes more often and see rejecting votes as opportunity for improvement. From my side, I can live with the current status of this SLEP. And we can later change based on further experience (law usually lags behind current developments). @NicolasHug As Gael said, in the meeting yesterday, we had the feeling that we should vote without specific technical points being discussed (therefore, you won't find anything). |
|
After sleeping it over it, I think it is important to try to get consensus on this specific SLEP because it is about SLEP workflow. We can try @NicolasHug's suggestion at #30 (comment) on this PR to see if it improves the workflow. I can help finish up addressing the concerns there and then push for a vote. Does that sound reasonable? |
|
This PR already has a few approvals, and as we discussed on the meeting, that counts as seconds to my call to vote. Therefore I'm merging this PR, and will call for a vote on the mailing list with an accompanying PR where we vote. As the author of the SLEP, I'm strongly opposed to the changes suggested in #30 (comment), and in fact, as already explained, moving away from that process is the main reason I drafted this SLEP in the first place. I'm happy to have improvements to this SLEP later, which will definitely happen. As to the main spirit of the suggested process, I think since both PEP and NEP follow the same process, we should try to do the same. If it's worked for them, it can work for us. |
There's no "moving away" from the process proposed in #30 (comment): we have never implemented this process before. We have however implemented the process suggested in this SLEP already (despite it not being merged nor voted upon), and from a personal perspective I can't say that it has been a tremendous success. Would you mind detailing precisely what parts of #30 (comment) you are against? This SLEP currently describes why our "current" workflow is less than optimal, but I haven't seen any specific feedback on #30 (comment). As a meta comment, the way we're moving forward here makes me loose confidence in our consensus seeking process. |
|
but I haven't seen any specific feedback on #30 (comment).
Some people have expressed disagreement on this comment.
the way we're moving forward here makes me loose confidence in our consensus seeking process.
Consensus sometimes requires compromise and accepting some things that we disagree with. I remember really struggling with such acceptance a few years ago, but now I feel that the group is stronger than the individual and if some decisions were really bad, many people in a talent group would see them as such. So a persisting disagreement probably reveals a grey zone.
|
I haven't been able to find anything specific on #30 (comment). I've seen disagreement on our current process (which I share), but it is very different from what is proposed in #30 (comment). |
|
@NicolasHug feel free to open a PR to edit the SLEP with what you suggest in: #30 (comment) to try to move forward and have a focused discussion on this. |
|
Thanks for the suggestion @ogrisel . Before I do that, I would prefer hearing opposing thoughts on #30 (comment) first. There is little value in submitting a PR while we know there are strong disagreements that haven't been explicitly stated yet. I will also wait until the end of the vote, as I'd prefer not doing this work for nothing. |
|
Personally I think the SLEP in its current state is better than no SLEP at all. I don't have an strong feeling on how to improve the github commenting workflow on SLEPs. Maybe the close/reopen strategy can work, but I think that the author should take the time to summarize unresolved discussion points in the description of the new SLEP PR or as inline comments. |
|
But also I think it's fine to close/reopen Draft SLEP PRs with a summary of the open discussion points without having to specify this process in details in SLEP 000. |
Gaël, I'm happy to compromise and I'm OK with accepting things that I disagree with. What is hard to swallow for me here is the lack of acknowledgement on the concerns we raised, and the lack of actionable feedback regarding the alternative proposals. We're just getting a rejection with no details, without being given any opportunity for improvement. A wise man once said make sure everyone feels heard. Sadly, I don't feel heard at all here. And the decision to merge this PR while it was still being actively discussed makes me believe that any potential future efforts will be met with the same indifference. |
There was a problem hiding this comment.
I like the idea in #30 (comment).
Alternative idea: keep commenting on merged proposals.
| to be made available as PR to the scikit-learn repo (making sure to | ||
| appropriately mark the PR as a WIP). | ||
|
|
||
| Review and Resolution |
There was a problem hiding this comment.
9 status are proposed here, but I am not sure which status would correspond to "Under review" or "Consolidated draft than is still under discussion". And what would be the current status of merged SLEP000 ? Should the SLEP be listed here ?
| change to (or an event in) a process. Process SLEPs are like Standards Track | ||
| SLEPs but apply to areas other than the scikit-learn library itself. They may | ||
| propose an implementation, but not to scikit-learn’s codebase; they require | ||
| community consensus. Examples include procedures, guidelines, changes to the |
There was a problem hiding this comment.
What does "community consensus" mean here?
|
What is hard to swallow for me here is the lack of acknowledgement on the concerns we raised, and the lack of actionable feedback regarding the alternative proposals. We're just getting a rejection with no details, without being given any opportunity for improvement.
I don't have a stake in this game. My impression at the meeting yesterday was that people felt that the SLEP could be improved, but that it was already an improvement on the existing and wanted to call a vote for this reason.
I did not feel a judgement on the specific concerns, more a wish to iterate. Things take a long time in scikit-learn :)
Also, I feel bad as the flag-bearer here, given that I am trying to express what I understood yesterday.
|
|
One last comment: I also resonate with a variety of comments that have been made. However, I'm feeling that the benefits of trying to reach a decision exceed those of further improvements, because these further improvements can come on top. I also think that with the current tooling there is not silver bullet. |
Triggered by the discussions we had during the Paris sprint, and and the recent discussions under #17 , I think it's time for us to have a common understanding of how a SLEP workflow should look like.
This PR heavily borrows (and copy/pastes) from PEP1 and NEP0, and adjusts where appropriate.
I believe it's a good start for us to have an easier and more enjoyable experience working on SLEPs.
cc @scikit-learn/core-devs