samuela packages: add mandatory PR notice

[samuela]

Description of changes

I frequently find that changes are made to packages that I maintain without my notice or consent. This behavior is intrusive and disruptive to maintenance, esp. considering that these changes are often poorly conceived. This PR simply adds a comment to the beginning of the files of packages that I maintain notifying contributors that changes made to these packages are expected to go through the usual PR process.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[samuela]

If no one objects I'll go ahead and merge tomorrow

[SuperSandro2000]

This has no support at all in the tooling used by the python ecosystem maintainers which gets used in python-updates runs. We could probably add support for it or just manually skip those packages. Nothing unsolvable.

But more importantly depending on how strict those packages pin their requirements this could make your maintainer situation (far) worse. Dependencies will still get updates and your packages will then just stay broken regardless if it would be easily fixable or not.

Also please keep in mind that you have really high standards for the unstable channel of a distributon. On Debian unstable and sometimes even on testing the dependencies of packages haven't been promoted yet, making it impossible to update/install some packages.

I suggested you already multiple ways how to collaborate more closely in the python-updates runs where you can always chime in and check if packages would break and suggest ways how to fix them (which can be reverts of update) ahead of them breaking on staging. So far I couldn't get you excited about that.

The only suggestions I got from your posts where that pythonPackages moves a lot slower or python-updates runs would stop. With the current maintainer participation in pythonPackages that won't work. Also I suspect that would move the maintenance burden more onto staging people. If you are looking for a slower moving channel I highly recommend you to consider switching to stable where breaking changes are rare. Or you could manage your python packages in a local project with poetry2nix where you can update entirely on our speed.

Finally I don't think this will make collaboration easier or better and currently I am against merging this as is.

[tpwrules]

I'm not happy with the phrasing of this message, but I very much sympathize with the intent. That said, it's not clear that this would have prevented any recent breakage because I don't see any recent objectionable commits to these files, except perhaps wandb's patch failure.

It is a truth that there are packages which require more care than average when touching, but the general expectation, which I think is good, is that non-maintainers touching packages is okay. An obvious note that touching a particular package may require lots of time, or particular hardware, or specific knowledge to test properly is very reasonable, along with a request to have someone familiar sign off. I'm just not convinced this is it.

If anything, you do need to clarify that by "maintainer" in the warning you mean somebody listed in the meta.maintainers attribute. I initially read it as thinking that e.g. one of the Python package set maintainers would be a suitable "maintainer" to approve changes, but I'm pretty sure now this is not your intention.

I've put some more thoughts on Matrix too if you want to talk more there.

[winterqt]

I'm not happy with the phrasing of this message, but I very much sympathize with the intent. That said, it's not clear that this would have prevented any recent breakage because I don't see any recent objectionable commits to these files, except perhaps wandb's patch failure.

Yeah, if there were a ton of instances where things broke for your packages because of you not being able to review things, I think my opinion on this may be different. I don't agree with this as a solution, because no other packages have it, and I don't think it helps a lot in the long run (I also think the wording is unnecessarily stern). Simple updates can usually be merged without requiring a maintainer's sign off, and any complex changes/things that people are unsure about usually should/do go through the maintainers.

I've put some more thoughts on Matrix too if you want to talk more there.

@tpwrules Were these in private? (I ask since I can't find any messages from you publicly.)

[tpwrules]

It was in the Nix CUDA room as samuela is a big part of the CUDA maintainers team, and CUDA/data science/ML/etc packages is where a lot of his frustration is centered I think. Those packages are universally complicated, fragile to package, and critical to daily operations. Nix being able to manage them is unbelievably helpful to those of us who work with them regularly, even if support is downgraded to only having an expectation of function on stable branches.

[jonringer]

I sympathize with the maintainer burdenship. A practical solution would just be to rename all of the files that you don't want to be automatically bumped to something other than default.nix, as we don't want to update packages that are purposefully pinned to a major (or minor) version.

[FRidh]

It is frustrating to see packages break over time, and it causes a headache also for me, having rather large environments of complex packages. Having your Python package in python-packages.nix means you get a lot of work done by others for free in the form of dependencies that are provided, but since the ecosystem is so brittle it also means it breaks often, as is the topic here.

I think you need to re-evaluate your expectations. We're talking about the master branch here which is considered unstable. Working with leaf packages that have many dependencies that change a lot and are brittle means you are at risk.

Going forward, I think what we can do is better indicate when large Python updates happen. I don't know how often they happen these days either, but it may be worth reducing how often they happen and perhaps even schedule them. Of course, people that want a rolling release will be unhappy then. Trade-offs.

[samuela]

If anything, you do need to clarify that by "maintainer" in the warning you mean somebody listed in the meta.maintainers attribute. I initially read it as thinking that e.g. one of the Python package set maintainers would be a suitable "maintainer" to approve changes, but I'm pretty sure now this is not your intention.

Good point! Fixed in 0e75f96.

[samuela]

Thanks everyone for all your comments. I never expected this change to garner so much attention. I just thought I'd touch on a few notes...

This PR was born out of frustrations from multiple incidents across many months. There was no single individual, breakage, or incident that prompted me to make this change. Instead, it has been more like a "death by a thousand paper cuts" story. I do not think that anyone has been engaging with ill intentions, and I do not take offense to any of the transgressing commits. Nevertheless in order to maintain sanity as a package maintainer, I require the ability to at least stay up-to-date with the changes being made to the code that I am responsible for.

Code reviews are a widely accepted best practice across the industry, not only because they catch bugs but because they are an essential form of information dissemination among people collaborating on a shared codebase. In Nixpkgs, we expect changes to go through the PR process and committing directly to master is strongly looked down upon. Why should staging/python-updates be any different?

Note that I am not opposed to being involved in python-updates. Rather I am never notified when changes in python-updates touch my packages. As a package maintainer I'm left in the dark. I am happy to review PRs targeting staging/python-updates. But please at least send me a PR.

[mweinelt]

On multiple occassions I came by the CUDA maintainers room on Matrix and mentioned that a python-updates run was commencing. That happened with the intention that all of you could take a look at the changes.
I explicitly mentioned you when I noticed jax/jaxlib needed an update to handle the numpy update, but you don't seem to be reachable via Matrix the last month.

Python-updates tooling touches all packages that have a default.nix below pkgs/development/python-modules/, see Jon's comment here: #185079 (comment). If there is a new version for any of these packages the tooling will touch it.

I think we talked about missing ofborg review requests on these PRs, which are likely due to the absolute number of maintainers exceeding the possible number of reviewers on the PR, which is 15 iirc.

[samuela]

The stability of master is somewhat orthogonal to the issue this PR is intended to address, and I have already recorded some of my thoughts on stability in "Nixpkgs’s current development workflow is not sustainable". But at the risk of engaging on this tangent, I'll just say: every time python-updates/staging gets merged into master a lot of packages I maintain/rely on are broken. And whenever I file issues or point out these build failures I hear the usual refrain "Your stability expectations are too high". How else should I participate to fix build failures? Should I not file issues? Should I not run my own CI? Should I not submit fixes?

I know that master is unstable. I am reminded of that every month or so. But every time I try to get these builds back to green, it feels like the response is "why? who cares if your builds aren't green?". It's exhausting.

[jonringer]

I know that master is unstable. I am reminded of that every month or so. But every time I try to get these builds back to green, it feels like the response is "why? who cares if your builds aren't green?". It's exhausting.

The sustainable way to address this is, why are there so many consistent failures. I generally try to upstream fixes when they are the idiomatic correct thing to do for a given ecosystem.

But I have also given up on certain projects like onnx-runtime as I don't think the culture and trajectory would ever align.

[piegamesde]

I am against this change. The comment adds nothing that should not be done everywhere already. I'd rather see some efforts to improve the situation nixpkgs-wide, and in a more sustainable way (i.e. some automation; comments are easily ignored).

I frequently find that changes are made to packages that I maintain without my notice or consent

Sadly, our pinging infrastructure is notoriously broken. I suggest that, as a workaround, you add yourself as CODEOWNER for all files related to packages that you maintain. This will at least guarantee pings on all relevant changes (including treewide refactorings that touch these packages, which you would not get otherwise).

[SuperSandro2000]

And whenever I file issues or point out these build failures I hear the usual refrain "Your stability expectations are too high". How else should I participate to fix build failures? Should I not file issues? Should I not run my own CI? Should I not submit fixes?

In the past you opened issues in an automated way. Sometimes multiple ones for the same build problem only for different consumers of the package. Please do a minimal amount of debugging and triaging before opening an issue. We don't need an issue for every (temporary) build problem. Also please include logs of the failure. Only linking the failed hydra job is not very time efficient if multiple people need to dig up the same log lines.

I am reminded of that every month or so.

Just to put that into perspective: I am reminded of that multiple times a week and because of that I feel like I started to maintain every other package listed in my configuration.

But I have also given up on certain projects like onnx-runtime as I don't think the culture and trajectory would ever align.

I get the same feeling for many AI/ML projects.

---

A sustainable way to fix this would be to move all the slow moving AI/ML packages that require specific version of dependencies out of nixpkgs into its own project. Then they can rely on older versions of packages without holding back nixpkgs and can do pins and overwrites to versions they require.

[samuela]

The comment adds nothing that should not be done everywhere already. I'd rather see some efforts to improve the situation nixpkgs-wide, and in a more sustainable way (i.e. some automation; comments are easily ignored).

Now, this is an idea I can get behind! However considering the unusual pushback that I'm receiving for only a handful of comments just on the packages I maintain, I don't see a path forward to doing this automatically and tree-wide, at least at the moment. That said, I'm absolutely open to ideas. Did you have a solution in mind?

[piegamesde]

My personal vision is to have functional tooling that pings all relevant people, and a merge bot system that allows more granular write access to this repository. Having so many people with write access to the entire repository does not scale well, and is causing problems (including yours). IMHO, the current meta.maintainers approach has failed and we should migrate to some file based code owner approach instead. See #143441 and #155440 for more discussion and experiments on this topic.

I'd like to expand on my "The comment adds nothing that should not be done everywhere already" from above. Specifically, I was referring to "Changes made to this file […] MUST be approved by a member of meta.maintainers before merging." In an ideal world, this would already be the case everywhere. However, many packages are simply unmaintained, or have unresponsive official maintainers and are effectively maintained by somebody else. Therefore, it is difficult to enforce this as a rule, and people have gotten used to work independently of the official maintainers. Some time, as a community, we will have to have a talk about what it means to be a nixpkgs package maintainer. We will need some proper expectation management and then also enforce rules that make sure that all packages are well maintained. I've already seen some occasional private conversations on this topic (you are far from being the only frustrated person) and I hope that they will lead to an RFC eventually. As a tangential, there is also NixOS/rfcs#127 which should lay the ground for automatically ensuring that all our packages are properly maintained.

---

I think it's best to close this PR, please try adding yourself in .github/CODEOWNERS instead. Let me know if this ends up not working well for you (preferably in #143441).

[samuela]

@piegamesde Thanks for you thoughtful response. It sounds like this is a known issue across the community.

I'm hesitant to go the CODEOWNERS route in this case because of all the failures brought up in #143441. What's the official doctrine on CODEOWNERS, meta.maintainers and the like? Is adding myself to CODEOWNERS mutually exclusive to the comment-based approach in this PR?

[SuperSandro2000]

I'm hesitant to go the CODEOWNERS route in this case because of all the failures brought up in #143441

Basically you might receive a notification to much when people do a bad rebase and force push and none for very big PRs like the staging ones. But it is the best tool we have right now and most of the time it works like it should.

What's the official doctrine on CODEOWNERS, meta.maintainers and the like?

Normally meta.maintainers should be enough but sometimes you have files where that does not work or want to receive notifications for an entire directory and then codeowners are used.

Is adding myself to CODEOWNERS mutually exclusive to the comment-based approach in this PR?

You can always add yourself to meta.maintainers and to codeowners if there is a compelling reason which I think we have here or meta.maintainers does not work. Going the comments approach was voted against earlier in this thread.

[piegamesde]

I'd like to add that the situation around CODEOWNERS has already improved a bit since I opened the issue.

• GitHub has a built-in lint for changes to the file that are semantically invalid
• We have figured out how to do rebasing without pinging everyone. People are still learning it, but the random pings I've gotten have definitely gotten less over time.