Skip to content

python3Packages.einops: fixed tests#152291

Closed
yl3dy wants to merge 1 commit intoNixOS:masterfrom
yl3dy:einops_fix
Closed

python3Packages.einops: fixed tests#152291
yl3dy wants to merge 1 commit intoNixOS:masterfrom
yl3dy:einops_fix

Conversation

@yl3dy
Copy link
Contributor

@yl3dy yl3dy commented Dec 26, 2021

Motivation for this change

Fix occasionally stuck tests, see #152049.

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.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@yl3dy yl3dy changed the title python3Packages.einops: fixed tests (#152049) python3Packages.einops: fixed tests Dec 26, 2021
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Dec 26, 2021
@veprbl veprbl linked an issue Dec 26, 2021 that may be closed by this pull request
python3Packages.einops tests are broken #152049
Closed
@yl3dy
Copy link
Contributor Author

yl3dy commented Dec 26, 2021

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

2 packages built:
  • python38Packages.einops
  • python39Packages.einops

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 26, 2021
risicle
risicle reviewed Dec 27, 2021
View reviewed changes
pkgs/development/python-modules/einops/default.nix Outdated
Copy link
Contributor

@risicle risicle Dec 27, 2021

Choose a reason for hiding this comment

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

I'm not sure you really have to bother tidying up after yourself or setting HOME back to its previous setting. Many packages set HOME in the checkPhase and I'm not sure I've ever seen any bother setting it back.

Copy link
Contributor

@risicle risicle Dec 27, 2021

Choose a reason for hiding this comment

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

Also, completely replaced checkPhases could do with runHook preCheck and runHook postCheck and the beginning and end respectively.

Copy link
Contributor

@legendofmiracles legendofmiracles Dec 27, 2021

Choose a reason for hiding this comment

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

I've been wondering about that, why is that needed? Does python add something to those phases, or is it for easier overriding?

Copy link
Contributor

@risicle risicle Dec 27, 2021

Choose a reason for hiding this comment

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

The hooks? AFAIK It's purely for the convenience of people who may need to override elements of the package and e.g. set their own extra preCheck, expecting it to work rather than being ignored.

Copy link
Contributor Author

@yl3dy yl3dy Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Many packages set HOME in the checkPhase and I'm not sure I've ever seen any bother setting it back.

It was more of a precaution, so removed that.

Also, completely replaced checkPhases could do with runHook preCheck and runHook postCheck and the beginning and end respectively.

Added, thanks a lot for the suggestion!

@yl3dy
python3Packages.einops: fixed tests (#152049)
4792183 
Tests now utilize a separate HOME created via `mktemp`: using just TMPDIR
makes `test_layers::test_torch_layer` sometimes fail.
@veprbl
Copy link
Member

veprbl commented Jan 4, 2022

@GrahamcOfBorg build python3Packages.einops

@veprbl
Copy link
Member

veprbl commented Jan 9, 2022

The tests seem to hang on test_torch_layer on ofborg and in my local tests. It seems like all tests are passing within 3m on the upstream github actions CI.

@yl3dy
Copy link
Contributor Author

yl3dy commented Jan 9, 2022

The tests seem to hang on test_torch_layer on ofborg and in my local tests. It seems like all tests are passing within 3m on the upstream github actions CI.

What system do you use? I tried building via nixpkgs-review tens of times with the proposed fixes on NixOS 21.11 x86_64 (don't have a darwin system unfortunately), and the whole test suite ran for less than 1m. I'm quite at loss here: tried my best to isolate tests, but don't have a lot of experience with PyTorch to determine what could possibly go wrong here. On the other hand, I'd rather not disable this test.

@veprbl
Copy link
Member

veprbl commented Jan 9, 2022

@yl3dy I was testing on non-nixos x86_64-linux running on an i7-3770K with sandboxing enabled.

@samuela
Copy link
Member

samuela commented Jan 10, 2022 •
edited
Loading

I just tried running nixpkgs-review on this PR and it's still hung up right after test_reduce_symbolic ... ok. I'm running an AWS EC2 m5.24xlarge instance. I believe this reproduces the issue that ofborg and @veprbl are running into.

❯ nix-shell -p nix-info --run "nix-info -m"
 - system: `"x86_64-linux"`
 - host os: `Linux 5.10.87, NixOS, 21.11 (Porcupine)`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.3.16`
 - channels(root): `"nixos-21.11.334578.75f3534c98a, nix-ld"`
 - channels(skainswo): `"home-manager, nixpkgs-unstable-22.05pre343295.adf7f03d3bf"`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`

I believe it should be reproducible running on any of the m5 instances (at least).

@yl3dy What kind of CPU are you running OOC? Anything else unusual about your setup?

@samuela samuela mentioned this pull request Jan 10, 2022
Merged
13 tasks
@yl3dy
Copy link
Contributor Author

yl3dy commented Jan 10, 2022

I'm running Core i7-12700K at NixOS 21.11 (stable channel), no dGPU. Running on CPU might be the most unusual, compared to non-Nix testing.

@yl3dy
Copy link
Contributor Author

yl3dy commented Jan 10, 2022

Closing in favor of #154115

@yl3dy yl3dy closed this Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@risicle risicle risicle left review comments

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

+1 more reviewer

@legendofmiracles legendofmiracles legendofmiracles left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

python3Packages.einops tests are broken

5 participants

@yl3dy @veprbl @samuela @risicle @legendofmiracles