Skip to content

Comments

python3Packages.treeo: 0.4.0 -> 0.0.9#159933

Closed
dotlambda wants to merge 2 commits intoNixOS:masterfrom
dotlambda:treeo-0.0.9
Closed

python3Packages.treeo: 0.4.0 -> 0.0.9#159933
dotlambda wants to merge 2 commits intoNixOS:masterfrom
dotlambda:treeo-0.0.9

Conversation

@dotlambda
Copy link
Member

@dotlambda dotlambda commented Feb 14, 2022

Motivation for this change

fixes #159455

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.

@dotlambda dotlambda requested review from ndl and samuela February 14, 2022 03:45
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Feb 14, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 14, 2022
@samuela
Copy link
Member

samuela commented Feb 14, 2022

Result of nixpkgs-review pr 159933 run on x86_64-linux 1

1 package failed to build:
  • python39Packages.elegy
3 packages built:
  • python310Packages.treeo
  • python39Packages.treeo
  • python39Packages.treex

samuela
samuela requested changes Feb 14, 2022
View reviewed changes
Copy link
Member

@samuela samuela left a comment

Choose a reason for hiding this comment

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

just confused re the version downgrade. o/w LGTM. if the 0.0.9 version is actually newer than 0.4.0, I think it my actually be preferable to just move to an unstable-yyyy-mm-dd version.

pkgs/development/python-modules/treeo/default.nix
# jax is not declared in the dependencies, but is necessary.
propagatedBuildInputs = [
jax
jaxlib
Copy link
Member

@samuela samuela Feb 14, 2022

Choose a reason for hiding this comment

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

Thank you! This should not have been in propagatedBuildInputs to begin with per https://nixos.wiki/wiki/JAX#How_do_I_package_JAX_libraries.3F.

Copy link
Contributor

@ndl ndl Feb 14, 2022

Choose a reason for hiding this comment

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

Wait, isn't that the opposite of what we should be doing here? IIUC it was correct before the change (= jaxlib used only in checkInputs and not in propagatedBuildInputs).

@dotlambda please keep it the way it was before (= not propagating jaxlib and explicitly passing it in checkInputs) as otherwise it forces the specific version of jaxlib onto the user, see the link above for details.

Copy link
Member

@samuela samuela Feb 14, 2022

Choose a reason for hiding this comment

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

oops! I was way too tired when I wrote that! yes jaxlib should absolutely not be in propagatedBuildInputs. Thanks for catching @ndl!

Copy link
Member

@SuperSandro2000 SuperSandro2000 Mar 29, 2022

Choose a reason for hiding this comment

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

Normally we use upstreams requirements as a good recommendations but many python packages use the wrong requires or force linters we don't want. If that package is required for this package to work correct then it can be propagated even if upstream has done something different.

Copy link
Member

@samuela samuela Mar 29, 2022

Choose a reason for hiding this comment

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

We have an established policy of not propagating jaxlib in the JAX ecosystem. Sometimes that means deviating from upstreams dependency lists, and creating issues upstream as well.

pkgs/development/python-modules/treeo/default.nix
buildPythonPackage rec {
pname = "treeo";
version = "0.4.0";
version = "0.0.9";
Copy link
Member

@samuela samuela Feb 14, 2022

Choose a reason for hiding this comment

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

From a semver perspective this is a downgrade. Why should we downgrade instead of upgrading?

Copy link
Contributor

@ndl ndl Feb 14, 2022

Choose a reason for hiding this comment

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

I see the point @dotlambda makes, it looks like upstream versioning failure indeed.

@samuela
Copy link
Member

samuela commented Feb 14, 2022

elegy build failure is unrelated and was already broken

@dotlambda
Copy link
Member Author

dotlambda commented Feb 14, 2022

just confused re the version downgrade. o/w LGTM. if the 0.0.9 version is actually newer than 0.4.0, I think it my actually be preferable to just move to an unstable-yyyy-mm-dd version.

The previous version was accidentally called 0.4.0 instead of 0.0.4. See https://github.com/cgarciae/treeo/releases and https://pypi.org/project/treeo/#history.
So 0.0.9 is the most recent version.
I don't see why we should deviate from upstream versioning.

@samuela
Copy link
Member

samuela commented Feb 14, 2022

Maybe I'm missing something but I don't see any indication that 0.4.0 was an accident in the release notes. Is there a reference for this?

@dotlambda
Copy link
Member Author

dotlambda commented Feb 14, 2022

Maybe I'm missing something but I don't see any indication that 0.4.0 was an accident in the release notes. Is there a reference for this?

Compare the version history on PyPI with the one on GitHub.

@ndl ndl mentioned this pull request Feb 14, 2022
Merged
10 tasks
@samuela
Copy link
Member

samuela commented Feb 14, 2022

I'm a little confused so I'll defer to maintainer @ndl on this one.

ndl
ndl suggested changes Feb 14, 2022
View reviewed changes
Copy link
Contributor

@ndl ndl left a comment

Choose a reason for hiding this comment

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

Using 0.0.9 instead of 0.4.0 looks the right step, thanks for catching the issue!

The only issue is with propagating jaxlib, please revert that part and I'll approve the rest.

pkgs/development/python-modules/treeo/default.nix
# jax is not declared in the dependencies, but is necessary.
propagatedBuildInputs = [
jax
jaxlib
Copy link
Contributor

@ndl ndl Feb 14, 2022

Choose a reason for hiding this comment

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

Wait, isn't that the opposite of what we should be doing here? IIUC it was correct before the change (= jaxlib used only in checkInputs and not in propagatedBuildInputs).

@dotlambda please keep it the way it was before (= not propagating jaxlib and explicitly passing it in checkInputs) as otherwise it forces the specific version of jaxlib onto the user, see the link above for details.

pkgs/development/python-modules/treex/default.nix
poetry-core
];

buildInputs = [ jaxlib ];
Copy link
Contributor

@ndl ndl Feb 14, 2022

Choose a reason for hiding this comment

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

Same as above - please keep it the way it is.

pkgs/development/python-modules/treeo/default.nix
buildPythonPackage rec {
pname = "treeo";
version = "0.4.0";
version = "0.0.9";
Copy link
Contributor

@ndl ndl Feb 14, 2022

Choose a reason for hiding this comment

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

I see the point @dotlambda makes, it looks like upstream versioning failure indeed.

@samuela samuela mentioned this pull request Feb 15, 2022
Closed
@samuela samuela mentioned this pull request Mar 29, 2022
Merged
13 tasks
@smancill
Copy link
Contributor

smancill commented Jul 23, 2022

Done by db60f46

@smancill smancill closed this Jul 23, 2022
@dotlambda dotlambda deleted the treeo-0.0.9 branch July 25, 2022 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuela samuela samuela requested changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

+1 more reviewer

@ndl ndl ndl requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

python3Packages.treex build failure on x86_64-linux as of 70aa8d9e

5 participants

@dotlambda @samuela @smancill @ndl @SuperSandro2000