Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
slep000, slep workflow #30
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
slep000, slep workflow #30
Changes from 2 commits
e8f5c9c
6d24c52
d296bcb
2d3fca6
bf2191f
fb32fa0
0e01bb4
4c42863
5155b22
845255b
10f1a80
77cc077
dbb936d
8ca1a82
888aabe
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is also the NEP process that seems fine, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is "earliest convenience"?
Should -> may (let's at least not make it a mandatory process please)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't believe this should be an obligation. But we don't have to agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To comment on this point: I feel that my view sits a bit in the middle of that of @NicolasHug and of @adrinjalali :
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for clarification, these are new, right? Does anyone want to give an example of these, and when they are useful? The notion is a bit vague for me tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we explicitly include the governance document or is that encapsulated in the decision-making process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "community consensus" mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, I think that we could add a section to give information about the scikit-learn version where the interface is considered stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xref: #56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SLEP in its current state says:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @adrinjalali I think the mechanisms described here should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.