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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

python311Packages.scalene: init at 1.5.38 #293107

Merged
2 commits merged into from
Mar 29, 2024
Merged

python311Packages.scalene: init at 1.5.38 #293107

2 commits merged into from
Mar 29, 2024

Conversation

sarahec
Copy link
Contributor

@sarahec sarahec commented Mar 3, 2024

Description of changes

Scalene is a high-performance CPU, GPU and memory profiler for Python that does a number of things that other Python profilers do not and cannot do. It runs orders of magnitude faster than many other profilers while delivering far more detailed information. It is also incorporates AI-powered optimization suggestions.

homepage URL: https://github.com/plasma-umass/scalene

closes #293086

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 馃憤 reaction to pull requests you find important.

Copy link
Member

@bcdarwin bcdarwin left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

pkgs/by-name/sc/scalene/package.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/scalene/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/scalene/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/scalene/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/scalene/default.nix Outdated Show resolved Hide resolved
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.

This should be two commits with appropriate commit messages, one for maintainer and one for the package.
Please also set meta.mainProgram.

pkgs/development/python-modules/scalene/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/scalene/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/scalene/default.nix Outdated Show resolved Hide resolved
@dotlambda dotlambda changed the title init: scalene at 1.5.35 python311Packages.scalene: init at 1.5.35 Mar 6, 2024
@sarahec
Copy link
Contributor Author

sarahec commented Mar 6, 2024

@dotlambda Thank you. I'll squash commits appropriately before this is done.

I'm working on switching it to a build from source w/ tests; that's going to take a bit.

@wamserma
Copy link
Member

wamserma commented Mar 6, 2024

Closes #293086

@sarahec
Copy link
Contributor Author

sarahec commented Mar 16, 2024

@bcdarwin and @dotlambda -- This has everything except a build from github source (as the latter is going to require altering the original's build system it seems.)

@sarahec
Copy link
Contributor Author

sarahec commented Mar 24, 2024

@bcdarwin and @dotlambda it's been a week since my last ping. Can we get this done?

@sarahec sarahec force-pushed the init-scalene branch 2 times, most recently from 81d536c to 2016299 Compare March 24, 2024 20:01
@sarahec sarahec changed the title python311Packages.scalene: init at 1.5.35 python311Packages.scalene: init at 1.5.38 Mar 24, 2024
@sarahec
Copy link
Contributor Author

sarahec commented Mar 24, 2024

Pushed update to 1.5.38 (with a Windows bugfix)

@sarahec
Copy link
Contributor Author

sarahec commented Mar 26, 2024

@bcdarwin Hi Ben, are you available to complete the review?

@sarahec
Copy link
Contributor Author

sarahec commented Mar 26, 2024

@dotlambda Hi Robert, are you available to complete the review?

Copy link
Contributor Author

@sarahec sarahec left a comment

Choose a reason for hiding this comment

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

Summary for new reviewers:

Scalene is a high-performance profiler for python that includes cpu, memory, and gpu.

This builds from PyPi-downloaded package.

Addressing the open requests

  1. The build is entirely from source , not incorporating the binary blobs from the wheel. meta.sourceProvenance does not need to be set.
  2. Building from the github source is infeasible due to the structure of setup.py -- it combines C header/source downloads and platform-specific compiles.
  3. The C headers and sources are included in the installable package so we are able to build from scratch. Only the unit tests are missing.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1561

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

looks good. need to enable tests but there are a few failures that need to be delt with.

diff to enable tests
diff --git a/pkgs/development/python-modules/scalene/default.nix b/pkgs/development/python-modules/scalene/default.nix
index eb77ed083342..ac2bf8975716 100644
--- a/pkgs/development/python-modules/scalene/default.nix
+++ b/pkgs/development/python-modules/scalene/default.nix
@@ -1,6 +1,7 @@
 { lib
 , buildPythonPackage
 , fetchPypi
+, fetchpatch
 , setuptools
 , setuptools-scm
 , cloudpickle
@@ -10,6 +11,9 @@
 , pynvml
 , pythonOlder
 , rich
+, pytestCheckHook
+, hypothesis
+, numpy
 }:
 
 buildPythonPackage rec {
@@ -23,6 +27,15 @@ buildPythonPackage rec {
     hash = "sha256-LR1evkn2m6FNBmJnUUJubesxIPeHG6RDgLFBHDuxe38=";
   };
 
+  patches = [
+    # fix scalene_config import. remove on next update
+    (fetchpatch {
+      name = "scalene_config-import-fix.patch";
+      url = "https://github.com/plasma-umass/scalene/commit/cd437be11f600ac0925ce77efa516e6d83934200.patch";
+      hash = "sha256-YjFh+mu5jyIJYUQFhmGqLXhec6lgQAdj4tWxij3NkwU=";
+    })
+  ];
+
   nativeBuildInputs = [
     cython
     setuptools
@@ -37,6 +50,20 @@ buildPythonPackage rec {
     rich
   ];
 
+  nativeCheckInputs = [
+    pytestCheckHook
+  ];
+
+  checkInputs = [
+    hypothesis
+    numpy
+  ];
+
+  # remove scalene directory to prevent pytest import confusion
+  preCheck = ''
+    rm -rf scalene
+  '';
+
   pythonImportsCheck = [ "scalene" ];
 
   meta = with lib; {

@sarahec
Copy link
Contributor Author

sarahec commented Mar 29, 2024

@a-n-n-a-l-e-e Thank you! Now, do you have an idea how I can enable or disable tests by platform? The failing tests (for me) are expecting Apple hardware.

EDIT: The scalene repo has removed these failing tests. I'll mark them as disabled with a note to remove on the next update.

@ghost
Copy link

ghost commented Mar 29, 2024

@a-n-n-a-l-e-e Thank you! Now, do you have an idea how I can enable or disable tests by platform? The failing tests (for me) are expecting Apple hardware.

the tests seem kind of broken given they are running on linux. i'd just disable them outright and report the issue upstream.
can disable all tests in a file via:

 disabledTestPaths = [
    "tests/test_coverup_35.py"
    "tests/test_coverup_42.py"
    "tests/test_coverup_43.py"
  ];

or just specific tests using disabledTests.

and to just use a platform can do

disabledTests = [ ... ] ++ lib.optionals stdenv.isDarwin [ disabled tests for darwin ];

@sarahec
Copy link
Contributor Author

sarahec commented Mar 29, 2024

I added

  disabledTestPaths = [
    # remove on next update
    # Failing Darwin-specific tests that were subsequently removed from the source repo.
    "tests/test_coverup_35.py"
    "tests/test_coverup_42.py"
    "tests/test_coverup_43.py"
  ];

as the failing tests are no longer in the source repo. There are also some test warnings; I'll file a PR against the scalene sources to fix.

@sarahec
Copy link
Contributor Author

sarahec commented Mar 29, 2024

That's all the changes done.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM. built on x64/aarch64 darwin and x64 linux.

@ghost ghost merged commit e283be8 into NixOS:master Mar 29, 2024
22 of 24 checks passed
@sarahec sarahec deleted the init-scalene branch March 29, 2024 18:26
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package request: scalene
5 participants