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

Already on GitHub? Sign in to your account

Jump to bottom

python311Packages.rlax: fix build #275438

Merged
merged 3 commits into from
Jan 9, 2024
Merged

python311Packages.rlax: fix build #275438

merged 3 commits into from
Jan 9, 2024

Conversation

GaetanLepage
Copy link
Contributor

Description of changes

An attempt at fixing plotnine to have bsuite and thus rlax working.

cc @onny

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.

@GaetanLepage GaetanLepage mentioned this pull request Dec 19, 2023
Merged
13 tasks
@ofborg ofborg bot requested review from onny and samuela December 19, 2023 21:11
samuela
samuela previously requested changes Dec 19, 2023
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.

looks like this is still a WIP, but my review was requested so providing some feedback in its current state

pkgs/development/python-modules/mizani/default.nix Show resolved Hide resolved
pkgs/development/python-modules/plotnine/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/plotnine/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/plotnine/default.nix Show resolved Hide resolved
pkgs/development/python-modules/rlax/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/rlax/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/rlax/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/rlax/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/rlax/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from samuela December 20, 2023 07:35
@imincik
Copy link
Contributor

imincik commented Jan 9, 2024

@GaetanLepage , great thanks for this PR. I see plotnine, bsuite and r-lax failing after all geospatial packages updates. I agree with all points raised by @samuela .

@GaetanLepage
Copy link
Contributor Author

@GaetanLepage , great thanks for this PR. I see plotnine, bsuite and r-lax failing after all geospatial packages updates. I agree with all points raised by @samuela .

Yes, I think that bsuite and rlax are only broken because of theuir dependency plotnine being itself broken.
I tried to have a look at the latter. Most of the tests pass (so no hard crash or similar error), but I was not able to understand why those tests were failing.
They are not failing when I run them on a non-Nix environment (using a venv).
Most of them consist in comparing generated images of plots to expected ones. Not really easy to debug as I can't see the generated results in the build environment.

@imincik
Copy link
Contributor

imincik commented Jan 9, 2024

Not really easy to debug as I can't see the generated results in the build environment.
You can try to interactively run the build using nix-develop-interactive.bash. Using this script you can manually run and re-run phases and you have full access to all files.

USAGE:

mkdir dev; cd dev
nix develop nixpkgs#<PACKAGE>
source nix-develop-interactive.bash

@GaetanLepage
Copy link
Contributor Author

You can try to interactively run the build using nix-develop-interactive.bash.

Thanks ! I wonder how I was not already aware of this tool.
So I was able to check.
The images are indeed different...by a minuscule amount. If you quickly cycle between expected and actual images, you see that a few elements are 1 pixel shifted.
Now, to know why this is happening... well I have no idea. Maybe some dependency being too old...

@GaetanLepage
Copy link
Contributor Author

I have disabled those tests.

@GaetanLepage
Copy link
Contributor Author

I have disabled those tests.

@GaetanLepage
Copy link
Contributor Author

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

16 packages built:
  • python310Packages.bsuite
  • python310Packages.bsuite.dist
  • python310Packages.mizani
  • python310Packages.mizani.dist
  • python310Packages.plotnine
  • python310Packages.plotnine.dist
  • python310Packages.rlax
  • python310Packages.rlax.dist
  • python311Packages.bsuite
  • python311Packages.bsuite.dist
  • python311Packages.mizani
  • python311Packages.mizani.dist
  • python311Packages.plotnine
  • python311Packages.plotnine.dist
  • python311Packages.rlax
  • python311Packages.rlax.dist

@GaetanLepage
python311Packages.mizani: 0.10.0 -> 0.9.3
f3ca2ad 
Downgrade as latest version plotnine (0.12.4) is not compatible with mizani==0.10.0
NickCao
NickCao approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@NickCao NickCao left a comment •
edited

Choose a reason for hiding this comment

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

diff LGTM, nixpkgs-review happy. Waiting for ofborg.

@GaetanLepage
Copy link
Contributor Author

Result of nixpkgs-review pr 275438 run on aarch64-linux 1

16 packages built:
  • python310Packages.bsuite
  • python310Packages.bsuite.dist
  • python310Packages.mizani
  • python310Packages.mizani.dist
  • python310Packages.plotnine
  • python310Packages.plotnine.dist
  • python310Packages.rlax
  • python310Packages.rlax.dist
  • python311Packages.bsuite
  • python311Packages.bsuite.dist
  • python311Packages.mizani
  • python311Packages.mizani.dist
  • python311Packages.plotnine
  • python311Packages.plotnine.dist
  • python311Packages.rlax
  • python311Packages.rlax.dist

imincik
imincik approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@imincik imincik left a comment

Choose a reason for hiding this comment

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

Great work, thanks !

@NickCao NickCao merged commit c033d82 into NixOS:master Jan 9, 2024
22 of 23 checks passed
@GaetanLepage GaetanLepage deleted the rlax branch January 9, 2024 20:13
@imincik imincik mentioned this pull request Jan 10, 2024
Merged
@ghost ghost added the backport release-23.11 Backport PR automatically label Feb 21, 2024
@github-actions github-actions bot mentioned this pull request Feb 21, 2024
Merged
1 task
@github-actions GitHub Actions
Copy link
Contributor

Successfully created backport PR for release-23.11:

@ghost ghost mentioned this pull request Feb 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imincik imincik imincik approved these changes

@NickCao NickCao NickCao approved these changes

@onny onny Awaiting requested review from onny

@samuela samuela Awaiting requested review from samuela

@natsukium natsukium Awaiting requested review from natsukium

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 backport release-23.11 Backport PR automatically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@GaetanLepage @imincik @samuela @NickCao @delroth @cole-h