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

python3Packages.twisted: remove dropin.cache files #174239

Draft
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

dotlambda
Copy link
Member

@dotlambda dotlambda commented May 24, 2022

Motivation for changes

They can lead to conflicts in python3.withPackages environments.
fixes #164775

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:
  • 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.05 Release Notes (or backporting 21.11 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.

Should this be backported?
cc @NixOS/release-engineers

@dotlambda
Copy link
Member Author

I don't know much about twisted. Maybe magic-wormhole-transit-relay shouldn't put its twisted plugins in $out/lib/python3.9/site-packages/twisted/plugins?

@dotlambda dotlambda force-pushed the twisted-remove-dropin-cache branch from d7a1a12 to 301b82a Compare May 24, 2022 06:53
They can lead to conflicts in python3.withPackages environments.
@piegamesde
Copy link
Member

Good question. I don't know much about twisted either, but FWIW the Arch packaging does the same. IIRC Twisted has some kind of plugin path lookup mechanism, so we could leverage that to make it find the plugin on some custom path, but not sure if this is a good idea.

@sarcasticadmin
Copy link
Member

@dotlambda Thanks for pushing this up!

I think this seems reasonable but I'll look at this a bit deeper later today.

@exarkun
Copy link
Contributor

exarkun commented May 24, 2022

They can lead to conflicts in python3.withPackages environments.

nixpkgs' python environments are complex and I'm not sure I understand them completely, however if these dropin.cache files are removed, every application that tries to load Twisted plugins will have to re-discover them all at runtime (each time) and then emit a warning about how it cannot write out a new dropin.cache file.

It seems to me that, instead, the dropin.cache files should be merged into the resulting environment.

Maybe this means it's actually withPackages job to write the file in the first place, instead of the job of each individual Python package?

@exarkun
Copy link
Contributor

exarkun commented May 24, 2022

(btw, I am a Twisted upstream dev, if nixpkgs wants Twisted's plugin cache to work differently so it works better in nixpkgs, feel free to talk to me about it)

@dotlambda
Copy link
Member Author

Maybe this means it's actually withPackages job to write the file in the first place, instead of the job of each individual Python package?

That's definitely something we could consider doing in the future. The same could happen when e.g. buildPythonApplication is used.

@dotlambda
Copy link
Member Author

if nixpkgs wants Twisted's plugin cache to work differently so it works better in nixpkgs, feel free to talk to me about it

One idea that immediately comes to mind is having a cache file per package, e.g. dropin.cache.d/twisted and dropin.cache.d/magic-wormhole-transit-relay. Each package would put its plugins in some kind of "namespace" that should usually match the package name.

I'm also interested in knowing how much of a slowdown not having the cache files actually is.

@exarkun
Copy link
Contributor

exarkun commented May 24, 2022

I'm also interested in knowing how much of a slowdown not having the cache files actually is.

It's probably more of a slowdown to not have and not be allowed to write it than just to not have it.

I don't think anyone has ever tried to benchmark the Twisted plugin system in a nixpkgs-like Python environment (which is very different from most non-nixpkgs Python environments - eg those created by Debian for system Python or by using virtualenv).

So - dunno. It could well be that the best thing for nixpkgs would be a way to just turn off the caching completely.

@exarkun
Copy link
Contributor

exarkun commented May 24, 2022

One idea that immediately comes to mind is having a cache file per package, e.g. dropin.cache.d/twisted and dropin.cache.d/magic-wormhole-transit-relay. Each package would put its plugins in some kind of "namespace" that should usually match the package name.

This sounds like how it already works in Twisted for the dropins themselves. For example, Twisted contributes a dropin named "twisted_web" by distributing twisted/plugins/twisted_web.py. magic-wormhole-transit-relay contributes a dropin named "magic_wormhole_transit_relay" by distributing `twisted/plugins/magic_wormhole_transit_relay.py".

Only when we get to the dropin cache file is there a conflict - and the conflict is by design. By putting the information for both "twisted_web" and "magic_wormhole_transit_relay" into "dropin.cache", the plugin system only has to read "dropin.cache" - not all of the other dropins in the directory.

If you namespaced the cache then I think the result would undo any possible gain you could receive from the cache existing in the first place.

@dotlambda
Copy link
Member Author

Maybe the best option for Nixpkgs is having something like twisted.withPackages that generates dropin.cache.

@SuperSandro2000
Copy link
Member

How important is that cache in terms of performance? Are we loosing seconds or a few ms? Does it actually matter when people run magic-wormhole or just for the tests? Can we make the hook so that it only runs in the final env? Would it make sense to invest time into building a hook which combines the different caches from different files which we before separated?

Something like:

  • move the dropin cache in postFixup to a location containing pname
  • combine them in the env again into one

@sarcasticadmin
Copy link
Member

@dotlambda this builds successfully for me given the following use case:

$ export NIX_PATH=nixpkgs=https://github.com/dotlambda/nixpkgs/archive/26e87ed448716fd5a245ad9bb9a2c8dfe0c11637.tar.gz
$ nix-build -E "with (import <nixpkgs> {}); python3.withPackages (pypkgs: with pypkgs; [magic-wormhole-transit-relay magic-wormhole-mailbox-server])"
# Putting this into a nix-shell also worked for me and calling something like `twist wormhole-mailbox`

Other thoughts on the thread:

Can we make the hook so that it only runs in the final env? Would it make sense to invest time into building a hook which combines the different caches from different files which we before separated?

Something like:

  • move the dropin cache in postFixup to a location containing pname
  • combine them in the env again into one

Could we make a hook for the final env that generates a new dropin.cache based on whats in the python env and then ensure that this dropin.cache is in the same env? if thats possible? It doesnt seem like twisted is too picky about where the dropin.cache is. But combining the existing caches seems to negate the benefits from what @exarkun mentioned. The docs mention how to create the dropin.cache1 as well:

from twisted.plugin import IPlugin, getPlugins
list(getPlugins(IPlugin))

How important is that cache in terms of performance? Are we loosing seconds or a few ms? Does it actually matter when people run magic-wormhole or just for the tests?

Good questions, I'm not particularly sure either. There's some hints from the docs around performance impacts, see below:

From the docs 1: While these applications will still run and find correct plugin information, they may run more slowly than they would if the cache was up to date, and they may also report exceptions if certain plugins have been removed but which the cache still references.

Footnotes

  1. https://docs.twistedmatrix.com/en/twisted-22.4.0/core/howto/plugin.html#plugin-caching 2

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 8, 2023
@dotlambda dotlambda closed this Jan 17, 2024
@dotlambda dotlambda deleted the twisted-remove-dropin-cache branch January 17, 2024 05:22
@dotlambda dotlambda restored the twisted-remove-dropin-cache branch January 17, 2024 05:26
@dotlambda dotlambda reopened this Jan 17, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 17, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
@wegank wegank marked this pull request as draft March 20, 2024 15:50
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

6 participants