PR to update packages:

• Set min version on pandas at 1.0.1 sebp/scikit-survival#100
• Update version to 0.6.2 osqp/osqp-python#42

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

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

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

• 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.

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!

@GrahamcOfBorg build python3Packages.scikit-survival

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

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 ?

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;
  };
}

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

Actually I can work on it this week

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

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

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?

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

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.

I've included your fix, thanks @drewrisinger

@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.

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

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

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

btw there's a new release available

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

thanks