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

python3Packages.word2vec: fix build #98143

Merged
merged 1 commit into from Sep 18, 2020
Merged

Conversation

@wkral
Copy link
Contributor

@wkral wkral commented Sep 17, 2020

Motivation for this change

ZHF: #97479
Testing changed to pytest, and requires an external dataset so check phase is omitted.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@wkral wkral requested review from FRidh and jonringer as code owners Sep 17, 2020
@wkral
Copy link
Contributor Author

@wkral wkral commented Sep 17, 2020

Result of nixpkgs-review pr 98143 1

2 packages built:
  • python37Packages.word2vec
  • python38Packages.word2vec
${python.interpreter} test_word2vec.py
'';
# Checks require test data that needs to be downloaded separately
doCheck = false;

This comment has been minimized.

@risicle

risicle Sep 17, 2020
Contributor

If you're going to disable the tests, could you at least add a pythonImportsCheck so we have some hope of detecting breakage?

(also, is there no subset of the tests which doesn't use the test dataset which you could enable?)

This comment has been minimized.

@wkral

wkral Sep 17, 2020
Author Contributor

Thanks I didn't know about pythonImportCheck.

The only test that was passing ironically was an import check so it didn't seem worth it. However, I tried downloading the support data to see if I could get that to work and it did. My worry with this version, is that hydra may impose some heavy bandwidth costs on a host that I'm unsure is equiped to handle them. Though, it's obviously better for ensuring the package works.

If this version looks good to you I'll squash the commits and push that up. Otherwise I'll just add the pythonImportCheck.

This comment has been minimized.

@risicle

risicle Sep 17, 2020
Contributor

How big is the support data?

This comment has been minimized.

@wkral

wkral Sep 17, 2020
Author Contributor

It's around 30 MiB, but the tests only use a small portion of that, notice the head command. The host is a personal website not a company or institution, which is why I hesitate. I expect the package devepers only really expect people to be downloading it infrequently when they setup the project on their dev machines for the first time.

Do you have some idea how often Hydra would end up downloading it? I don't think it maintains a cache for stuff like that.

Maybe I could just check in the test data file? It's only 100KB zipped it's 33KB. I suppose there are licencing implications with that though.

This comment has been minimized.

@risicle

risicle Sep 17, 2020
Contributor

I think hydra does keep a cache of these files.

This comment has been minimized.

@risicle

risicle Sep 17, 2020
Contributor

And a 33K test file would almost definitely be a no-go. We're even trying to keep patch files out of the repo these days.

@risicle
Copy link
Contributor

@risicle risicle commented Sep 17, 2020

This package helpfully has gcc hardcoded into its build system, so to get this working (again?) on macos, you'll need to provide pkgs.gcc in the nativeBuildInputs. That having been said, when I do so, I still get 3 test failures on macos 10.14.

@wkral
Copy link
Contributor Author

@wkral wkral commented Sep 17, 2020

No problem, I'll add gcc, looks like it's in the python code, so yeah can't just override it with make flags.

I still get 3 test failures on macos 10.14.

Which 3 are those? Maybe I can figure out why.

@risicle
Copy link
Contributor

@risicle risicle commented Sep 17, 2020

https://gist.github.com/risicle/a34a7c1e6105b52fdb7dd819ce6c38dd

It's possible that these are only failing because the mac I have access to is managed and has a few random annoying restrictions added by the IT team 🙄 .

@wkral
Copy link
Contributor Author

@wkral wkral commented Sep 17, 2020

The last two failures are a result of the first failure with the -6 return code. Which apparently means the word2vec sub process aborted. Could be a memory allocation issue, but I'm not too sure.

@wkral wkral force-pushed the wkral:word2vec-fix-build branch from 85145f4 to 27ce8e6 Sep 18, 2020
@wkral
Copy link
Contributor Author

@wkral wkral commented Sep 18, 2020

Result of nixpkgs-review pr 98143 1

2 packages built:
  • python37Packages.word2vec
  • python38Packages.word2vec
@wkral
Copy link
Contributor Author

@wkral wkral commented Sep 18, 2020

It's not clear to me when it last worked on macOS, the latest build on trunk show it hitting the missing gcc issue. So I've fixed that, and if there is a cache for the intermeidate build inputs then I doubt the bandwidth will be too bad. So I squashed all the commits.

Happy to improve things if there is another suggestion on how to make it work on macOS, but maybe it would even work on one of the hydra builders given your potential IT issues.

Copy link
Contributor

@jonringer jonringer left a comment

LGTM

Result of nixpkgs-review pr 98143 1

2 packages built:
  • python37Packages.word2vec
  • python38Packages.word2vec

@jonringer jonringer merged commit 6a4e785 into NixOS:master Sep 18, 2020
17 checks passed
17 checks passed
tests tests
Details
action
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="27ce8e6"; rev="27ce8e67b52761084fc255d26937f5e05ab56fa6"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="27ce8e6"; rev="27ce8e67b52761084fc255d26937f5e05ab56fa6"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="27ce8e6"; rev="27ce8e67b52761084fc255d26937f5e05ab56fa6"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="27ce8e6"; rev="27ce8e67b52761084fc255d26937f5e05ab56fa6"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="27ce8e6"; rev="27ce8e67b52761084fc255d26937f5e05ab56fa6"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="27ce8e6"; rev="27ce8e67b52761084fc255d26937f5e05ab56fa6"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="27ce8e6"; rev="27ce8e67b52761084fc255d26937f5e05ab56fa6"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@risicle
Copy link
Contributor

@risicle risicle commented Sep 18, 2020

Yup, a step in the right direction 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.