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

test: add test for grpcio wheel usage #899

Merged
merged 9 commits into from
Feb 27, 2023

Conversation

cpcloud
Copy link
Collaborator

@cpcloud cpcloud commented Dec 17, 2022

Adds a test to demonstrate failure to use the grpcio 1.51.1 osx 12.0 wheel available on PyPI wheel at all.

Fixes #842.

@cpcloud
Copy link
Collaborator Author

cpcloud commented Dec 18, 2022

@adisbladis I could really use your help here, it seems like preferWheel has no effect anymore. #842 is likely the same issue demonstrated in this PR.

Curiously, throwing maturin in as a dependency the preferWheel attribute is evaluated.

Any idea why this would be the case?

@cpcloud
Copy link
Collaborator Author

cpcloud commented Dec 18, 2022

It seems like reordering the overlays in overrides.withDefaults put the user overlay before the default poetry overrides overlay achieves the desired behavior. Admittedly, this is not a solution that I arrived at from thorough analysis.

My rationale was that it seemed like eval isn't happening at all on many packages, so I tried "forcing" eval by putting user overrides first in the list of overlays.

@cpcloud cpcloud force-pushed the grpcio-wheel branch 2 times, most recently from 768b7f5 to 1984b1c Compare December 18, 2022 12:44
@cpcloud
Copy link
Collaborator Author

cpcloud commented Dec 18, 2022

@asymmetric Can you try this PR out on your panel use case? I've incorporated a test for it here, and I think I've addressed the issue of preferWheel not ever being evaluated.

@cpcloud cpcloud force-pushed the grpcio-wheel branch 2 times, most recently from 3e1e2f5 to 6f80667 Compare December 19, 2022 14:06
@asymmetric
Copy link
Contributor

asymmetric commented Dec 25, 2022

@cpcloud thanks for looking into this!

We actually don't use withDefaults, we do:

      mkPoetryApplication = {
        projectDir = ./.;
        python = pythonVersion;
        overrides = [
          pkgs.poetry2nix.defaultPoetryOverrides
          (final: prev: {
            panel = prev.panel.override { preferWheel = true; };
            bokeh = prev.bokeh.override { preferWheel = true; };
          })
          # more custom overlays
        ];
      };

I moved the inline overlay above pkgs.poetry2nix.defaultPoetryOverrides, and it indeed solves the issue!

One thing I don't understand though is why the -A preferWheel test passes even without this PR.

@cpcloud cpcloud requested a review from adisbladis January 7, 2023 11:38
@cpcloud
Copy link
Collaborator Author

cpcloud commented Jan 12, 2023

@adisbladis Any chance you can take a look at this PR?

tests/grpcio-no-wheel/pyproject.toml Outdated Show resolved Hide resolved
tests/grpcio-no-wheel/pyproject.toml Outdated Show resolved Hide resolved
tests/grpcio-wheel/pyproject.toml Show resolved Hide resolved
tests/grpcio-no-wheel/default.nix Outdated Show resolved Hide resolved
tests/grpcio-wheel/default.nix Outdated Show resolved Hide resolved
@asymmetric
Copy link
Contributor

One thing I don't understand though is why the -A preferWheel test passes even without this PR.

@cpcloud do you have any ideas about this?

@@ -0,0 +1,18 @@
{ lib, poetry2nix, python3, pkgs, runCommand }:
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing: what does this test catch that the panel/bokeh one doesn't?

Could we just limit ourselves to one of the two? If not, then maybe adding an explanation of what exactly is being tested could be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, grpcio fails because of a missing wheel on the latest GitHub macos runners.

@adisbladis adisbladis merged commit f33815c into nix-community:master Feb 27, 2023
@cpcloud cpcloud deleted the grpcio-wheel branch February 27, 2023 02:59
@yorickvP
Copy link
Contributor

Applying defaults after user overlays feels wrong to me, and it does in fact break my code. It prevents users from undoing changes made in the default overlays.
If preferWheel doesn't compose right that's a deeper issue that we should fix.

Please revert this.

Code that breaks:

poetry2nix.overrides.withDefaults (super: self: {
  pytest = null;
})

@cpcloud
Copy link
Collaborator Author

cpcloud commented Feb 27, 2023

@adisbladis @yorickvP's request seems reasonable. Shall we revert?

@adisbladis
Copy link
Member

I'm OK with a revert.

@asymmetric
Copy link
Contributor

Any ideas about what the underlying cause of the lack of composability of preferWheel?

@yorickvP
Copy link
Contributor

Yes, .override is from the callPackage call in mk-poetry-dep.nix, while overridePythonAttrs comes from its call to buildPythonPackage, which doesn't know about the outer callPackage.

Options:

  • override overridePythonAttrs to know about the outer callPackage.
  • change buildPythonPackage to take a function, just like mkDerivation these days, and specify preferWheel through overridePythonAttrs
  • remove preferWheel somehow
  • ???

@iwanb
Copy link
Contributor

iwanb commented Jul 4, 2023

Applying defaults after user overlays feels wrong to me, and it does in fact break my code. It prevents users from undoing changes made in the default overlays. If preferWheel doesn't compose right that's a deeper issue that we should fix.

Please revert this.

Code that breaks:

poetry2nix.overrides.withDefaults (super: self: {
  pytest = null;
})

I encountered the same issue. Will the change be reverted? As a workaround I did:

overrides = [ poetry2nix.defaultPoetryOverrides  ...];

sigprof added a commit to sigprof/nix-devenv-qmk that referenced this pull request Feb 1, 2024
In nix-community/poetry2nix#899 the order of applying user and default
overrides had been changed to apply user overrides first; this breaks
overriding packages which already have entries in the default overrides.
As a workaround, build the list of override overlays manually instead of
using `poetry2nix.overrides.withDefaults`.
@sigprof
Copy link
Contributor

sigprof commented Feb 1, 2024

This also breaks in cases like #1519, or when trying to apply a local override for rpds-py while #1502 is still pending. The workaround in #899 (comment) helps, but why then have overrides.withDefaults if it can't be used sanely?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

preferWheel = true ignored for package panel
7 participants