scikit-survival: init at 0.11 #82410

GuillaumeDesforges · 2020-03-12T14:11:37Z

Motivation for this change

Add scikit-survival to nixpkgs

https://github.com/sebp/scikit-survival

Things done

Motivation

scikit-survival was missing from nixpkgs

Notes

Had to PR the package because of requirements issues.
Will put back src on actual source when PR is merged.
See sebp/scikit-survival#98

GuillaumeDesforges · 2020-03-18T14:05:59Z

PR to update packages:

pkgs/development/python-modules/scikit-survival/default.nix

pkgs/development/python-modules/osqp/default.nix

wd15 · 2020-03-23T16:56:36Z

You'll probably need to collapse these 5 commits into 2, each with the correct commit message. Builds for me though.

Result of nixpkgs-review pr 82410 1

7 package failed to build:

python27Packages.osqp
python37Packages.cvxpy
python37Packages.osqp
python37Packages.scikit-survival
python38Packages.cvxpy
python38Packages.osqp
python38Packages.scikit-survival

drewrisinger

Main suggestion is fetchFromGitHub, PR numbers, and note about reverting OSQP changes after version is bumped. Haven't tested build locally.

pkgs/development/python-modules/osqp/default.nix

pkgs/development/python-modules/scikit-survival/default.nix

GuillaumeDesforges · 2020-03-23T18:31:18Z

Thanks for the reviews. I'll work on it asap.

Fix points raised by drewrisinger
- fetchFromGitHub
- Typos
- Formatting
Fix builds for Python != 3.7

drewrisinger · 2020-03-24T13:25:50Z

Fix builds for Python != 3.7

If you're referring to @wd15 's failed builds above, those are probably due to an unfree dependency (mkl) of osqp not working with automated PR review.

GuillaumeDesforges · 2020-03-24T14:47:44Z

If you're referring to @wd15 's failed builds above, those are probably due to an unfree dependency (mkl) of osqp not working with automated PR review.

Didn't think of that. Will check for Python 2 still, but thanks I'll keep that in mind!

jonringer · 2020-03-27T21:11:46Z

pkgs/development/python-modules/osqp/default.nix

-    sha256 = "130frig5bznfacqp9jwbshmbqd2xw3ixdspsbkrwsvkdaab7kca7";
+  src = fetchgit {
+    url = "https://github.com/GuillaumeDesforges/osqp-python";
+    rev = "770ecc3b96a94b30344cf3721a2e0771ef5c8259";


Suggested change

rev = "770ecc3b96a94b30344cf3721a2e0771ef5c8259";

rev = "770ecc3b96a94b30344cf3721a2e0771ef5c8259";

fatal: reference is not a tree: 770ecc3b96a94b30344cf3721a2e0771ef5c8259 Unable to checkout 770ecc3b96a94b30344cf3721a2e0771ef5c8259 from https://github.com/GuillaumeDesforges/osqp-python. cannot build derivation '/nix/store/z2c2zviawy62qbkzaqx4pbs44xpm4a2m-python2.7-osqp-unstable-2020-03-18.drv': 1 dependencies couldn't be built

FRidh · 2020-03-29T12:29:34Z

@GrahamcOfBorg build python3Packages.scikit-survival

sikmir · 2020-07-28T08:55:16Z

pkgs/development/python-modules/scikit-survival/default.nix

+  meta = with lib; {
+    description = "Survival analysis built on top of scikit-learn";
+    homepage = "https://github.com/sebp/scikit-survival";
+    license = licenses.gpl3;


licenses.gpl3 is deprecated, consider using licenses.gpl3Only or licenses.gpl3Plus.

alexvorobiev · 2021-04-13T19:09:42Z

According to Pypi, the current version is "0.15.0.post0". Should this PR be updated?

GuillaumeDesforges · 2021-04-14T08:22:53Z

Sorry, I've left it aside for too long. I don't have the time to update it right now. Could you take over @alexvorobiev ?

alexvorobiev · 2021-04-20T19:51:46Z

I have crude (e.g. tests disabled, etc.) nix derivations for about 20 non-nixpkgs python packages so this one would be at the end of the queue... Here is what I ended up doing for the latest version of scikit-survival:

{ lib
, buildPythonPackage
, fetchPypi
, ecos
, joblib
, numexpr
, numpy
, osqp
, pandas
, scipy
, scikitlearn
, cython
}:

buildPythonPackage rec {
  pname = "scikit-survival";
  version = "0.15.0.post0";

  src = fetchPypi {
    inherit pname version;
    sha256 = "572c3ac6818a9d0944fc4b8176eb948051654de857e28419ecc5060bcc6fbf37";
  };

  buildInputs = [
    cython
  ];

  propagatedBuildInputs = [
    ecos
    joblib
    numexpr
    numpy
    osqp
    pandas
    scipy
    scikitlearn
  ];

  doCheck = false;
  doInstallCheck = false;

  meta = with lib; {
    description = "Survival analysis built on top of scikit-learn";
    homepage = https://github.com/sebp/scikit-survival;
    license = licenses.gpl3Plus;
  };
}

drewrisinger · 2021-04-20T20:04:08Z

I have crude (e.g. tests disabled, etc.) nix derivations for about 20 non-nixpkgs python packages so this one would be at the end of the queue... Here is what I ended up doing for the latest version of scikit-survival:

Mostly LGTM. Maybe open a separate PR for that?

Comments:

Tests, obviously
Needs maintainer
quotes on homepage URL
Add changelog if exists

GuillaumeDesforges · 2021-04-21T09:04:44Z

Actually I can work on it this week

GuillaumeDesforges · 2021-04-25T22:51:29Z

Tests fail, e.g.

$ nix-build . -A python3Packages.scikit-survival
...
_____________________ ERROR collecting tests/test_tree.py ______________________
ImportError while importing test module '/build/scikit-survival-0.15.0.post0/tests/test_tree.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/nix/store/q6gfck5czr67090pwm53xrdyhpg6bx67-python3-3.8.9/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/test_tree.py:13: in <module>
    from sksurv.tree import SurvivalTree
sksurv/tree/__init__.py:1: in <module>
    from .tree import SurvivalTree  # noqa: F401
sksurv/tree/tree.py:15: in <module>
    from ._criterion import LogrankCriterion
E   ModuleNotFoundError: No module named 'sksurv.tree._criterion'

Basically, it seems it does not cythonize. If I do

$ nix-shell . -A python3Packages.scikit-survival
[nix-shell] $ genericBuild

I can see that the .pyx do not get cynthonize (no .py files in build output, hence the import errors).

The setup.py does have a specific mechanism for extension, but editing it to display the list of extensions seem to show that they are defined and go through the cynthonize call.

I'm a but lost here, it's beyond what I know about nixpkgs/python/cython

drewrisinger · 2021-04-26T22:36:33Z

The setup.py does have a specific mechanism for extension, but editing it to display the list of extensions seem to show that they are defined and go through the cynthonize call.

They definitely go through the cythonize call, as I observed while watching the build just now. They're even installed into the output /nix/store/... path:

I suspect this is a similar issue I had while packaging python3Packages.qiskit-terra. I think that pytest gets confused about the proper directory for the scikit-survival code, looks in /build/... for the cythonized files and doesn't find them, and then errors.

A few suggestions:

Add an import check for the cythonized file: pythonImportsCheck = [ ... "sksurv.tree._criterion" ];
Resolve the tests issue: Nominally, you might be able to force pytest to check the local directory last (pytestFlagsArray = [ "--import-mode=append"];), but I've had more luck copying the tests to a new directory and running pytest from it. E.g. https://github.com/drewrisinger/nur-packages/blob/7fbf633d8820eee1543b01b1a615d8ad37eb87ce/pkgs/python-modules/qiskit-aer/default.nix#L119-L128

GuillaumeDesforges · 2021-04-27T08:20:47Z

I think that pytest gets confused about the proper directory for the scikit-survival code, looks in /build/... for the cythonized files and doesn't find them, and then errors.

@drewrisinger thanks for sharing your fix, I'll try that :)

But isn't that a bigger issue that would need some more global fix in nixpkgs rather than a script to fix it per package? Or is it really package-specific? I mean, won't a lot of cythonized packages fail like this one in the future?

pkgs/development/python-modules/scikit-survival/default.nix

SuperSandro2000 · 2021-04-27T13:52:49Z

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

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

2 packages failed to build and are new build failures:

python38Packages.scikit-survival: plain log | pretty log
python39Packages.scikit-survival: log was empty

drewrisinger · 2021-04-27T19:15:35Z

But isn't that a bigger issue that would need some more global fix in nixpkgs rather than a script to fix it per package? Or is it really package-specific? I mean, won't a lot of cythonized packages fail like this one in the future?

Not sure. Possibly. It seems to be restricted to packages that use both cython & pytest for tests, maybe also ones that run their tests only from the /build/source dir. This is only the second package (other than the Qiskit ones) that I've run across that's had the issue, so not sure if it's worth coming up with a universal solution.

EDIT: I just reviewed another PR where this is an issue #120909. I think you're right @GuillaumeDesforges that we need to start coming up with a better automatic solution.

GuillaumeDesforges · 2021-04-28T09:44:38Z

I've included your fix, thanks @drewrisinger

drewrisinger · 2021-04-28T12:18:32Z

@GuillaumeDesforges I've been running nix-review for about 45 mins, and the tests seem to be taking most of the runtime. Would you be able to figure out some of the slowest tests and disable them? The point of the nix tests isn't to comprehensively unittest the software, more to check that it isn't obviously broken, and I personally feel like it isn't worth the computational time running full tests for "leaf" packages like this which will likely only have a few users.

I suggestion:

running the build temporarily with pytestFlagsArray = [ ... "--durations=20" ];, nix-build -A python3Packages.scikit-survival
disable any obviously slow tests (i.e. disabledTests = [ "test_name_1" "test_name_2" ]. I commonly use a threshold of ~10 seconds, but up to you.

GuillaumeDesforges · 2021-04-28T12:46:35Z

I personally feel like it isn't worth the computational time running full tests for "leaf" packages like this

Agreed 100%, hadn't thought about it. I'll patch it asap

drewrisinger

Minor style points on diff
Commit LGTM
Builds via nix-review on x86-64-linux:

2 packages built:
python38Packages.scikit-survival python39Packages.scikit-survival

pkgs/development/python-modules/scikit-survival/default.nix

drewrisinger

All comments addressed, LGTM. Tested w/ nix-review again, ran fine.

2 packages built:
python38Packages.scikit-survival python39Packages.scikit-survival

GuillaumeDesforges · 2021-05-03T12:29:47Z

Thanks @drewrisinger for the review :) who should I poke for merge?

drewrisinger · 2021-05-03T12:57:36Z

pinging @SuperSandro2000 for merge. I don't have merge permissions

Artturin · 2021-08-12T01:55:05Z

btw there's a new release available

marsam · 2021-08-12T02:31:37Z

This caused evaluation errors when aliases are disabled. Fixed in #133576

Artturin · 2021-08-12T02:40:59Z

thanks

GuillaumeDesforges requested review from FRidh and jonringer as code owners March 12, 2020 14:11

GuillaumeDesforges mentioned this pull request Mar 12, 2020

Remove version on osqp, set min version on pandas sebp/scikit-survival#98

Closed

ofborg bot added 6.topic: python 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Mar 12, 2020

GuillaumeDesforges mentioned this pull request Mar 18, 2020

Set min version on pandas at 1.0.1 sebp/scikit-survival#100

Closed

GuillaumeDesforges force-pushed the pr/scikit-survival/init branch from fbe54dc to a5e2bc0 Compare March 18, 2020 13:55

ofborg bot requested a review from drewrisinger March 18, 2020 14:08

jonringer reviewed Mar 18, 2020

View reviewed changes

GuillaumeDesforges force-pushed the pr/scikit-survival/init branch from 9f6c5e0 to e7fd1bb Compare March 18, 2020 22:10

GuillaumeDesforges requested a review from jonringer March 19, 2020 21:52

drewrisinger suggested changes Mar 23, 2020

View reviewed changes

jonringer reviewed Mar 27, 2020

View reviewed changes

sikmir reviewed Jul 28, 2020

View reviewed changes

ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020

GuillaumeDesforges force-pushed the pr/scikit-survival/init branch from fa778e4 to a1d68b7 Compare April 25, 2021 22:45

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 25, 2021

SuperSandro2000 reviewed Apr 27, 2021

View reviewed changes

pkgs/development/python-modules/scikit-survival/default.nix Outdated Show resolved Hide resolved

GuillaumeDesforges requested a review from jonringer April 27, 2021 10:18

drewrisinger mentioned this pull request Apr 27, 2021

bpython: fix build #120909

Merged

10 tasks

GuillaumeDesforges force-pushed the pr/scikit-survival/init branch from fe54485 to 0032221 Compare April 28, 2021 09:43

GuillaumeDesforges force-pushed the pr/scikit-survival/init branch from 0032221 to cb1c59a Compare April 28, 2021 09:45

GuillaumeDesforges force-pushed the pr/scikit-survival/init branch from cb1c59a to 5f38890 Compare April 28, 2021 16:59

GuillaumeDesforges requested a review from drewrisinger April 28, 2021 17:00

drewrisinger approved these changes Apr 30, 2021

View reviewed changes

pkgs/development/python-modules/scikit-survival/default.nix Outdated Show resolved Hide resolved

pkgs/development/python-modules/scikit-survival/default.nix Outdated Show resolved Hide resolved

scikit-survival: init at 0.11

80748d2

GuillaumeDesforges force-pushed the pr/scikit-survival/init branch from 5f38890 to 80748d2 Compare May 3, 2021 11:56

GuillaumeDesforges requested a review from drewrisinger May 3, 2021 11:56

drewrisinger approved these changes May 3, 2021

View reviewed changes

drewrisinger added the needs_merger label May 3, 2021

Artturin merged commit c108863 into NixOS:master Aug 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scikit-survival: init at 0.11 #82410

scikit-survival: init at 0.11 #82410

GuillaumeDesforges commented Mar 12, 2020 •

edited

Loading

GuillaumeDesforges commented Mar 18, 2020

wd15 commented Mar 23, 2020

drewrisinger left a comment

GuillaumeDesforges commented Mar 23, 2020 •

edited

Loading

drewrisinger commented Mar 24, 2020

GuillaumeDesforges commented Mar 24, 2020

jonringer Mar 27, 2020

FRidh commented Mar 29, 2020

sikmir Jul 28, 2020

alexvorobiev commented Apr 13, 2021

GuillaumeDesforges commented Apr 14, 2021

alexvorobiev commented Apr 20, 2021

drewrisinger commented Apr 20, 2021

GuillaumeDesforges commented Apr 21, 2021

GuillaumeDesforges commented Apr 25, 2021 •

edited

Loading

drewrisinger commented Apr 26, 2021

GuillaumeDesforges commented Apr 27, 2021

SuperSandro2000 commented Apr 27, 2021

drewrisinger commented Apr 27, 2021 •

edited

Loading

GuillaumeDesforges commented Apr 28, 2021

drewrisinger commented Apr 28, 2021 •

edited

Loading

GuillaumeDesforges commented Apr 28, 2021

drewrisinger left a comment •

edited

Loading

drewrisinger left a comment

GuillaumeDesforges commented May 3, 2021

drewrisinger commented May 3, 2021

Artturin commented Aug 12, 2021

marsam commented Aug 12, 2021

Artturin commented Aug 12, 2021

	rev = "770ecc3b96a94b30344cf3721a2e0771ef5c8259";
	rev = "770ecc3b96a94b30344cf3721a2e0771ef5c8259";

scikit-survival: init at 0.11 #82410

scikit-survival: init at 0.11 #82410

Conversation

GuillaumeDesforges commented Mar 12, 2020 • edited Loading

Motivation for this change

Things done

Motivation

Notes

GuillaumeDesforges commented Mar 18, 2020

wd15 commented Mar 23, 2020

drewrisinger left a comment

Choose a reason for hiding this comment

GuillaumeDesforges commented Mar 23, 2020 • edited Loading

drewrisinger commented Mar 24, 2020

GuillaumeDesforges commented Mar 24, 2020

jonringer Mar 27, 2020

Choose a reason for hiding this comment

FRidh commented Mar 29, 2020

sikmir Jul 28, 2020

Choose a reason for hiding this comment

alexvorobiev commented Apr 13, 2021

GuillaumeDesforges commented Apr 14, 2021

alexvorobiev commented Apr 20, 2021

drewrisinger commented Apr 20, 2021

GuillaumeDesforges commented Apr 21, 2021

GuillaumeDesforges commented Apr 25, 2021 • edited Loading

drewrisinger commented Apr 26, 2021

GuillaumeDesforges commented Apr 27, 2021

SuperSandro2000 commented Apr 27, 2021

drewrisinger commented Apr 27, 2021 • edited Loading

GuillaumeDesforges commented Apr 28, 2021

drewrisinger commented Apr 28, 2021 • edited Loading

GuillaumeDesforges commented Apr 28, 2021

drewrisinger left a comment • edited Loading

Choose a reason for hiding this comment

drewrisinger left a comment

Choose a reason for hiding this comment

GuillaumeDesforges commented May 3, 2021

drewrisinger commented May 3, 2021

Artturin commented Aug 12, 2021

marsam commented Aug 12, 2021

Artturin commented Aug 12, 2021

GuillaumeDesforges commented Mar 12, 2020 •

edited

Loading

GuillaumeDesforges commented Mar 23, 2020 •

edited

Loading

GuillaumeDesforges commented Apr 25, 2021 •

edited

Loading

drewrisinger commented Apr 27, 2021 •

edited

Loading

drewrisinger commented Apr 28, 2021 •

edited

Loading

drewrisinger left a comment •

edited

Loading