-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
python37Packages.tensorflow-bin_2: fix tensorflow-bin_2 on mac #116496
python37Packages.tensorflow-bin_2: fix tensorflow-bin_2 on mac #116496
Conversation
fa5110c
to
e30b872
Compare
The only package that evals for me is And the python37 package then fails to build with:
|
Thanks for the review @SuperSandro2000. I apologize for it not being cleaner. I was working off an old master before rebasing 285b260. It looks like this package is now broken on master (4307699) becase httplib2 has test failures. |
e30b872
to
e44be29
Compare
I fixed the remaining version conflict issues and the ignored the failing network related tests in the upstream packages. These upstream packages mostly had some mocked local webserver tests. It is interesting that these were building on Linux, but not Mac. Does the linux sandboxing somehow allow for local network based tests? |
Also let me know if I should split the commits into separate PRs. I was trying to minimize the red tape. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise LGTM
@@ -28,6 +29,8 @@ buildPythonPackage rec { | |||
pytestCheckHook | |||
]; | |||
|
|||
pytestFlagsArray = lib.optionals stdenv.isDarwin ["-k 'not test_run_local_server'"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pytestFlagsArray = lib.optionals stdenv.isDarwin ["-k 'not test_run_local_server'"]; | |
disabledTests = lib.optionals stdenv.isDarwin [ "test_run_local_server" ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to the google-auth-oauthlib commit.
Most of the tests were failing on Mac OS, and tests were only recently enabled for this package.
e44be29
to
bf8b375
Compare
Okay. I replied to the reviews. I modified the relevant commits with |
Thats fine :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try __darwinAllowLocalNetworking = true;
?
This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch). Result of 38 packages marked as broken and skipped:
2 packages failed to build and already failed to build on hydra master:
103 packages built:
|
I did not. Not sure how to do that. |
@SuperSandro2000 Does this PR require any more action from me? |
@jonringer @SuperSandro2000 This fixes several important python packages on darwin, and your last review comments, which I addressed, were mostly of an aesthetic nature...I would’ve been okay with you manually applying those changes when I had your attention. This is my first contribution so would appreciate some feedback. Would splitting this into multiple PRs have made it easier for you? Just wondering for future contributions...seems like “I made some small change and merged your PR” is pretty common in the guix mailing lists, but if each iteration in nixpkgs takes weeks before re-review it puts a lot of emphasis on simplifying the PRs I think. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
I am pretty sure this resolves #108614 |
That is correct. I reported #108614, now after applying the change |
This disables some tests that using a mocked webserver.
These tests fail in a sandboxed build on mac
This package was broken on mac with some wildcarding issues. Some more constraints in the tensorflow wheel needed to be relaxed.
43c7cd7
to
5542995
Compare
See
|
Thanks. @SuperSandro2000. I was just confused bc you "approved" the PR and then asked a question w/o saying it was a requirement for merging. I didn't know what you wanted me to do nor why. I would've appreciated some reasoning for this since |
This package was broken on mac with some wildcarding issues.
Some more constraints in the tensorflow wheel needed to be relaxed.
This is my first PR, so I apologize if I am missing anything.
Motivation for this change
The package was broken, and the source builds don't work on Mac see e.g.: https://discourse.nixos.org/t/tensorflow-on-20-09-mac/11931/2.
I plan to backport this to 20.09.
Resolves #116364.
Closes #108614
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)I did import tensorflow in python and try some simple operations.