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.

Sign up for GitHub

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

Jump to bottom

python3Packages.scikitlearn: rename to scikit-learn #123017

Merged
merged 2 commits into from May 17, 2021
Merged

python3Packages.scikitlearn: rename to scikit-learn #123017

merged 2 commits into from May 17, 2021

Conversation

DavHau
Copy link
Member

@DavHau DavHau commented May 14, 2021

Motivation for this change

The name on pypi for this package is scikit-learn but it is named scikitlearn in nixpkgs

Things done

Correct the name while staying downwards conpatible.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@r-rmcgibbo
Copy link

r-rmcgibbo commented May 14, 2021 •
edited

Result of nixpkgs-review pr 123017 at 0351e84f run on aarch64-linux 1

2 packages built successfully:
  • python38Packages.scikit-learn
  • python39Packages.scikit-learn
2 suggestions:
  • warning: python-include-tests

    Consider adding a checkPhase for tests, or if not feasible, pythonImportsCheck.

    Near pkgs/development/python-modules/scikit-learn/default.nix:69:0:

       |
69 |     description = "A set of python modules for machine learning and data mining";
   | ^
  • warning: maintainers-missing

    Package does not have a maintainer. Consider adding yourself?

    Near pkgs/development/python-modules/scikit-learn/default.nix:78:5:

       |
78 |     maintainers = with maintainers; [ ];
   |     ^

Result of nixpkgs-review pr 123017 at 0351e84f run on x86_64-linux 1

2 packages built successfully:
  • python38Packages.scikit-learn
  • python39Packages.scikit-learn
1 suggestion:
  • warning: maintainers-missing

    Package does not have a maintainer. Consider adding yourself?

    Near pkgs/development/python-modules/scikit-learn/default.nix:78:5:

       |
78 |     maintainers = with maintainers; [ ];
   |     ^

@rmcgibbo
Copy link
Contributor

rmcgibbo commented May 14, 2021 •
edited

Should we also rename its usage spots in all downstream packages within nixpkgs at the same time?

@dotlambda dotlambda changed the title scikitlearn: rename to scikit-learn python3Packages.scikitlearn: rename to scikit-learn May 14, 2021
@dotlambda dotlambda added the 9.needs: maintainer Package or module needs active maintainers label May 14, 2021
dotlambda
dotlambda requested changes May 15, 2021
View reviewed changes
pkgs/top-level/python2-packages.nix Outdated Show resolved Hide resolved
@DavHau
Copy link
Member Author

DavHau commented May 15, 2021

Committed suggestion and also enabled parallel building and testing

dotlambda
dotlambda requested changes May 15, 2021
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Can you squash your commits into two commits: one for the renaming and one for enabling the tests?

pkgs/top-level/python2-packages.nix Outdated Show resolved Hide resolved
@DavHau DavHau force-pushed the davhau-scikitlearn branch 2 times, most recently from 0bc84ce to eedd37d Compare May 15, 2021 08:54
@DavHau
Copy link
Member Author

DavHau commented May 15, 2021

squashed commits and removed scikitlearn alias.

dotlambda
dotlambda reviewed May 15, 2021
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

I fixed the commit messages and removed a commit involving prosody.

pkgs/development/python-modules/scikit-learn/default.nix Outdated
doCheck = !stdenv.isAarch64;
# Skip test_feature_importance_regression - does web fetch
checkPhase = ''
cd $TMPDIR
HOME=$TMPDIR OMP_NUM_THREADS=1 pytest -k "not test_feature_importance_regression" --pyargs sklearn
HOME=$TMPDIR OMP_NUM_THREADS=1 pytest -n $(( $(nproc) + 1 )) -k "not test_feature_importance_regression" --pyargs sklearn
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
HOME=$TMPDIR OMP_NUM_THREADS=1 pytest -n $(( $(nproc) + 1 )) -k "not test_feature_importance_regression" --pyargs sklearn
HOME=$TMPDIR OMP_NUM_THREADS=1 pytest -n $NIX_BUILD_CORES -k "not test_feature_importance_regression" --pyargs sklearn

Copy link
Member Author

Choose a reason for hiding this comment

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

done

pkgs/development/python-modules/scikit-learn/default.nix Outdated
@@ -54,17 +55,24 @@ buildPythonPackage rec {
joblib
threadpoolctl
];
checkInputs = [ pytest ];

checkInputs = [ pytest pytest-xdist ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
checkInputs = [ pytest pytest-xdist ];
checkInputs = [ pytestCheckHook pytest-xdist ];

You'll have to use disabledTests and pytestFlagsArray.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@DavHau DavHau force-pushed the davhau-scikitlearn branch 2 times, most recently from b333677 to dcb4483 Compare May 17, 2021 11:00
@DavHau DavHau requested a review from dotlambda May 17, 2021 11:01
dotlambda
dotlambda reviewed May 17, 2021
View reviewed changes
pkgs/development/python-modules/scikit-learn/default.nix Outdated
# Skip test_feature_importance_regression - does web fetch
checkPhase = ''
disabledTests = "test_feature_importance_regression";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
disabledTests = "test_feature_importance_regression";
disabledTests = [ "test_feature_importance_regression" ];

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@DavHau DavHau requested a review from dotlambda May 17, 2021 12:32
@dotlambda
Copy link
Member

@ofborg build python38.pkgs.scikit-learn python39.pkgs.scikit-learn

@dotlambda
Copy link
Member

On Darwin, the tests also failed before this change, so that can be fixed another time.

@dotlambda dotlambda merged commit a22ebb6 into NixOS:master May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmcgibbo rmcgibbo rmcgibbo left review comments

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@dotlambda dotlambda Awaiting requested review from dotlambda

Assignees
No one assigned
Labels
6.topic: nixos 6.topic: python 8.has: clean-up 8.has: module (update) 8.has: package (new) 9.needs: maintainer Package or module needs active maintainers 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@DavHau @r-rmcgibbo @rmcgibbo @dotlambda