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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pythonPackages.kaldi-active-grammar: fix build #203485

Merged
merged 2 commits into from
Dec 3, 2022
Merged

Conversation

ckiee
Copy link
Member

@ckiee ckiee commented Nov 28, 2022

Description of changes

Fixes #203089; cc @Lykos153 @mweinelt.

Snuck backports-zoneinfo in as it's an indirect dependency and something I ran into because my test shell had python38. Can drop.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.05 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

ZHF: #199919

@ckiee ckiee added 0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign backport release-22.11 labels Nov 28, 2022
@ckiee ckiee changed the title pythonPackages.backports-zoneinfo: fix build pythonPackages.kaldi-active-grammar: fix build Nov 28, 2022

# commit f07a0615ea82351bbddc0799364774cb350ff759 "openblas: 0.3.20 -> 0.3.21"
# ... breaks header compatibility with the outdated kaldi-active-grammar tree.
old-openblas = openblas.overrideAttrs (prev: rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird things might happen if this package were imported alongside numpy

Copy link
Member Author

Choose a reason for hiding this comment

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

Action recommended? It's contained to just this derivation so I don't anticipate much fallout.

Copy link
Contributor

@risicle risicle Nov 28, 2022

Choose a reason for hiding this comment

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

Took a bit of digging but revealed opencv having similar issues:

  * Use LAPACK_xxxx to transparently (via predefined lapack macros) deal with pre and post 3.9.1 versions.
  * LAPACK 3.9.1 introduces LAPACK_FORTRAN_STRLEN_END and modifies (through preprocessing) the declarations of the following functions used in opencv
  *        sposv_, dposv_, spotrf_, dpotrf_, sgesdd_, dgesdd_, sgels_, dgels_
  * which end up with an extra parameter.
  * So we also need to preprocess the function calls in opencv coding by prefixing them with LAPACK_.
  * The good news is the preprocessing works fine whatever netlib's LAPACK version.

https://github.com/opencv/opencv/blob/9777fbacf6799512551b937362249aa24ca07ce2/cmake/OpenCVFindLAPACK.cmake#L59

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding

    substituteInPlace src/matrix/cblas-wrappers.h \
      --replace stptri_ LAPACK_stptri \
      --replace dtptri_ LAPACK_dtptri \
      --replace sgetrf_ LAPACK_sgetrf \
      --replace dgetrf_ LAPACK_dgetrf \
      --replace sgetri_ LAPACK_sgetri \
      --replace dgetri_ LAPACK_dgetri \
      --replace sgesvd_ LAPACK_sgesvd \
      --replace dgesvd_ LAPACK_dgesvd \
      --replace ssptri_ LAPACK_ssptri \
      --replace dsptri_ LAPACK_dsptri \
      --replace ssptrf_ LAPACK_ssptrf \
      --replace dsptrf_ LAPACK_dsptrf

to postPatch allows it to build successfully with 0.3.21

Copy link
Member

Choose a reason for hiding this comment

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

Same with openfst. We should not merge this in the current state.

Copy link
Contributor

Choose a reason for hiding this comment

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

The openfst one isn't new and I think it's much less likely to produce a collision - numpy is everywhere on the other hand.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed @risicle's suggestion; @SuperSandro2000: KAG uses a openfst fork, so it's not the same.

(I tried typing this with the working build and it does work, but effort. Spent like two minutes figuring out I need to say "location <500ms> say <200ms> super <500ms> tab" or a longer string)

Copy link
Member

Choose a reason for hiding this comment

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

KAG uses a openfst fork, so it's not the same.

has it the same import path? If so they are considered the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also tried it before saying that and it does not work. The fork is based on 1.7, we are on 1.8 - I am not interested in debugging this, and it's out of the scope of my changes: cleaning [openfst] up.

Copy link
Contributor

@risicle risicle left a comment

Choose a reason for hiding this comment

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

Builds happily, aarch64-linux

@risicle risicle merged commit 930c705 into NixOS:master Dec 3, 2022
@risicle
Copy link
Contributor

risicle commented Dec 3, 2022

Might be worth suggesting the openblas fix to upstream.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2022

Successfully created backport PR #204282 for release-22.11.

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.

kaldi-active-grammar: "cannot load library"
3 participants