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

NEP 36 (fair play) #14779

Merged
merged 10 commits into from
Oct 17, 2020
Merged

NEP 36 (fair play) #14779

merged 10 commits into from
Oct 17, 2020

Conversation

stefanv
Copy link
Contributor

@stefanv stefanv commented Oct 25, 2019

This is to get the ball rolling on the "rules of engagement" NEP.

@stefanv stefanv changed the title Add initial draft of NEP 36 WIP: Add initial draft of NEP 36 Oct 25, 2019
@stefanv
Copy link
Contributor Author

stefanv commented Oct 25, 2019

Can anyone else who was on the call remind me of other ideas that came up?

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Looks like a good start, thanks Stefan.

We may want the language to be a bit more general, so other community packages can adopt this same policy.

doc/neps/nep-0036-fair-play.rst Outdated Show resolved Hide resolved
doc/neps/nep-0036-fair-play.rst Outdated Show resolved Hide resolved
doc/neps/nep-0036-fair-play.rst Outdated Show resolved Hide resolved
doc/neps/nep-0036-fair-play.rst Show resolved Hide resolved
doc/neps/nep-0036-fair-play.rst Outdated Show resolved Hide resolved
@stefanv
Copy link
Contributor Author

stefanv commented Oct 29, 2019

Thanks for the feedback, @rgommers, will incorporate that in here.

Copy link
Contributor

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

A few minor editorial changes.

doc/neps/nep-0036-fair-play.rst Outdated Show resolved Hide resolved
doc/neps/nep-0036-fair-play.rst Outdated Show resolved Hide resolved
Copy link

@amueller amueller left a comment

Choose a reason for hiding this comment

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

I think this is a good start. I think we should discuss and link to the broader OSS community. Does Apache have protocols for this? How about the Software Sustainability Institute? This seems like a question that any vendored project would have to face, and I would be surprised if bash, git and ssl didn't have these issues.

doc/neps/nep-0036-fair-play.rst Outdated Show resolved Hide resolved
doc/neps/nep-0036-fair-play.rst Outdated Show resolved Hide resolved
include the phrase `numpy`, i.e., avoid names such as
`mycompany_numpy`.

NumPy is a trademark owned by NumFOCUS.

Choose a reason for hiding this comment

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

which means using these names probably constitutes a trademark violation in the US, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

which means using these names probably constitutes a trademark violation in the US, right?

Good point, but I'm not sure adding stronger language would necessarily be an improvement. E.g. something like:

NumPy is a trademark owned by NumFOCUS. Unauthorized use of the NumPy name or logo may constitute a violation of this trademark.

but I'd be hesitant to add wording that invites legal discussions in the NEP itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am following up on the NumPy trademark.

@stefanv stefanv changed the title WIP: Add initial draft of NEP 36 WIP: NEP 36 (fair play) Nov 18, 2019
@mattip mattip added the triage review Issue/PR to be discussed at the next triage meeting label Feb 19, 2020
@mattip mattip removed the triage review Issue/PR to be discussed at the next triage meeting label Feb 26, 2020
@mattip
Copy link
Member

mattip commented Jul 13, 2020

@stefanv this has been dormant for a while. Should we accept it as a draft to get the ball rolling?

@stefanv
Copy link
Contributor Author

stefanv commented Jul 13, 2020

@mattip Let me at least integrate the feedback received here. Feel free to ping me if this hasn't happened by the end of the week.

@rossbar
Copy link
Contributor

rossbar commented Oct 13, 2020

@rgommers it looks like some of the initial wording suggestions were addressed in cd475fb - just a gentle ping to get your feedback on the updates when you have the time!

@stefanv stefanv changed the title WIP: NEP 36 (fair play) NEP 36 (fair play) Oct 14, 2020
@stefanv
Copy link
Contributor Author

stefanv commented Oct 14, 2020

I read the document again, and I think it's reasonable as a start. We can iterate on and extend it as necessary.

Comment on lines +20 to +22
Companies and developers will know after reading this NEP what kinds
of behavior the community would like to see, and which we consider
troublesome, bothersome, and unacceptable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Companies and developers will know after reading this NEP what kinds
of behavior the community would like to see, and which we consider
troublesome, bothersome, and unacceptable.
We identify acceptable approaches from which both parties gain, and unacceptable ones that lead to confusion and wasted effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's more to it than this. It is not only that there will be confusion and wasted effort, but that this will antagonize the developers. And not only because they have to do more work (no-one here seems too scared of that), but because it goes against the philosophy of the project and does not feel like participation in good faith.

Copy link
Contributor

Choose a reason for hiding this comment

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

Those clearly worded sentiments belong in the NEP; "troublesome, bothersome, and unacceptable" is not the equivalent.

Using my wording a strawman, the NEP can say

We identify acceptable approaches from which both parties gain, and unacceptable ones that lead to confusion and wasted effort and go against the philosophy of the project.

Insert your preferred wording; antagonism and bad faith can be folded in too.

At any rate I'd urge against the -some words. Their tone doesn't work in the intended way; they look like they're waiting for "drat!".

@rgommers
Copy link
Member

Looks close to ready. Let's merge as draft once the last key issue is addressed. I'll let @stefanv merge the textual changes from @bjnath he agrees with.

@bjnath
Copy link
Contributor

bjnath commented Oct 16, 2020

If you absolutely have to break this rule

We need to explain "absolutely have to" so everyone is on the same page about what the circumstances might be. For instance, list what people ought to have tried before resorting to this, or what cases are permissible (is that what the paragraph after this is meant to illustrate?).

Also, the sentence has two "absolutely"s ("absolutely have to," "absolutely clear") and might read better if one were replaced.

Similarly, we should clarify the "absolutely have to" in rule 3.

@rgommers
Copy link
Member

We need to explain "absolutely have to" so everyone is on the same page about what the circumstances might be. For instance, list what people ought to have tried before resorting to this, or what cases are permissible (is that what the paragraph after this is meant to illustrate?).

@bjnath all circumstances cannot sensibly be predicted. The current text seems perfectly fine, I suggest not over-rotating on the clarity requests here. The point of a NEP like this is to express ourselves clearly enough that anyone working on NumPy gets the message, and comes to talk to us if they think their use case falls under this "absolutely".

@bjnath
Copy link
Contributor

bjnath commented Oct 16, 2020

Also, we ought to circle back to the case of the security issue, because a similar case might recur. The distros acted swiftly to protect users. What do we say they should have done?

@rgommers
Copy link
Member

Also, we ought to circle back to the case of the security issue, because a similar case might recur. The distros acted swiftly to protect users. What do we say they should have done?

Nothing, they did what they had to here. Once a CVE is created for a security issue, distros have to do something about it, no matter if the CVE is justified or not.

@bjnath
Copy link
Contributor

bjnath commented Oct 16, 2020

@rgommers

Also, we ought to circle back to the case of the security issue, because a similar case might recur. The distros acted swiftly to protect users. What do we say they should have done?

Nothing, they did what they had to here. Once a CVE is created for a security issue, distros have to do something about it, no matter if the CVE is justified or not.

We've presented it like a negative example.

  1. We've said it led to API divergence. How did we resolve that, or did we never resolve that?
  2. The reason for bringing it up might be clarified. "This was a case where the distributions did what they had to, but..."

@bjnath
Copy link
Contributor

bjnath commented Oct 16, 2020

@rgommers

and comes to talk to us

If talking to us is an essential part of the requirement, I don't think that's made sufficiently clear here. We told them earlier to post to the list, but we need to repeat it here (similarly in the next bullet), so nobody thinks the version tag thing alone is sufficient.

stefanv and others added 2 commits October 16, 2020 15:51
@stefanv
Copy link
Contributor Author

stefanv commented Oct 16, 2020

The main intent around the security issue is to communicate that it leads to an undesirable state of things. That is not always available, but if we all communicate we can at least coordinate our response.

@bjnath
Copy link
Contributor

bjnath commented Oct 17, 2020

@stefanv @rgommers

if we all communicate we can at least coordinate our response

As a hypothetical distro maker, I'm confused about expectations. @rgommers says to fix first and talk later; @stefanv seems to say talk first. The NEP itself says nothing.

The NEP needs to say what we expect distros to do in security instances like these, because we raise the issue and then fail to give guidance.

@bjnath
Copy link
Contributor

bjnath commented Oct 17, 2020

@stefanv

will be considered good faith efforts in line with the
expectations of the NumPy developers.

Nice fix!

Nit: Per Google style, spell the compound modifier "good-faith," with a hyphen.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

I re-read the whole NEP, it's in pretty good shape and expresses intent and desired/undesired behaviour clearly. This PR has already been open for too long, let's not make it a full year - I'll merge this as is. Thanks @stefanv.

Two comments for a follow-up, which I hope will follow soon so we can get this NEP to "Accepted" state:

@rgommers rgommers merged commit 2ce4ab4 into numpy:master Oct 17, 2020
@rgommers
Copy link
Member

As a hypothetical distro maker, I'm confused about expectations. @rgommers says to fix first and talk later; @stefanv seems to say talk first. The NEP itself says nothing.

It's "do both". Security issues are rare and dealt with by a very small audience, details do not belong in this NEP.

@stefanv
Copy link
Contributor Author

stefanv commented Oct 18, 2020

I've opened a new PR for those final changes.

Thanks for reviewing and merging!

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.

None yet

10 participants