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

Jump to bottom

pythonPackages.imgaug: 0.3.0-patch #67494

Merged
merged 1 commit into from Jan 7, 2020
Merged

pythonPackages.imgaug: 0.3.0-patch #67494

merged 1 commit into from Jan 7, 2020

Conversation

Rakesh4G
Copy link
Contributor

@Rakesh4G Rakesh4G commented Aug 26, 2019 •
edited

Added patch to bypass opencv-python dependency from imgaug

@CMCDragonkai

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @CMCDragonkai @FRidh

@Rakesh4G Rakesh4G changed the title imgaug: 0.2.9-patch pythonPackages.imgaug: 0.2.9-patch Aug 26, 2019
@Rakesh4G
Copy link
Contributor Author

Rakesh4G commented Aug 26, 2019 •
edited

package has been updated as per inputs from

#61936

aleju/imgaug#379 (comment)

@CMCDragonkai
Copy link
Member

Note that in the master branch of imgaug, the author has changed to opencv-python-headless from opencv-python. See: https://github.com/aleju/imgaug/blob/master/setup.py#L21.

This means for this release 0.2.9, it should work, but the next release may require a change to the substitute patch.

@CMCDragonkai CMCDragonkai mentioned this pull request Aug 27, 2019
Closed
CMCDragonkai
CMCDragonkai approved these changes Aug 27, 2019
View reviewed changes
Copy link
Member

@CMCDragonkai CMCDragonkai left a comment •
edited

Choose a reason for hiding this comment

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

Approved, looks good. Mark it ready for review.

@CMCDragonkai CMCDragonkai mentioned this pull request Aug 27, 2019
Closed
@Rakesh4G Rakesh4G marked this pull request as ready for review August 27, 2019 05:03
@Rakesh4G Rakesh4G requested a review from FRidh as a code owner August 27, 2019 05:03
@mmahut
Copy link
Member

mmahut commented Aug 27, 2019

@GrahamcOfBorg build python37Packages.imgaug

@mmahut
Copy link
Member

mmahut commented Aug 27, 2019

Looks like resulting derivation is empty?

[nix-shell:~/.cache/nix-review/pr-67494/results/python37Packages.imgaug]# tree
.
└── nix-support
    └── propagated-build-inputs

1 directory, 1 file

@ofborg ofborg bot requested a review from CMCDragonkai September 6, 2019 11:38
@Rakesh4G Rakesh4G mentioned this pull request Sep 12, 2019
Closed
@CMCDragonkai
Copy link
Member

This is blocked on aleju/imgaug#414

@tbenst tbenst mentioned this pull request Nov 7, 2019
Closed
10 tasks
@veprbl veprbl added the 2.status: wait-for-upstream Waiting for upstream fix (or their other action). label Nov 7, 2019
@Rakesh4G Rakesh4G changed the title pythonPackages.imgaug: 0.2.9-patch pythonPackages.imgaug: 0.3.0-patch Nov 13, 2019
@jonringer
Copy link
Contributor

do you mind rebasing on top of master? apparently there's some merge conflicts:

git pull -r origin master
git push <fork> <branch> --force

@Rakesh4G
Copy link
Contributor Author

do you mind rebasing on top of master? apparently there's some merge conflicts:

git pull -r origin master
git push <fork> <branch> --force

Sure, I will.

@tbenst
Copy link
Contributor

tbenst commented Nov 25, 2019

Given that imgaug is functioning on waiting on upstream for a fix, perhaps we should merge & fix broken package?

@Rakesh4G
Copy link
Contributor Author

Rakesh4G commented Dec 3, 2019 •
edited

The tests hang issue aleju/imgaug#414 with imgaug-0.3.0 has been fixed on master. I have taken the latest changes and current expression is ready to be merged.
@tbenst @CMCDragonkai @jonringer

@CMCDragonkai
Copy link
Member

When attempting to install the latest changes using nix-env -f ./default.nix -iA 'pkgs.python3Packages.imgaug, the tests for hang here:

test/augmenters/test_meta.py::TestAugmenter_augment_batches::test_augment_batches_list_of_list_of_invalid_datatype PASSED

The build just stops there, the pytest command is still running but with 0% CPU usage.

@Rakesh4G
Copy link
Contributor Author

Rakesh4G commented Dec 11, 2019 •
edited

nix-env -f ./default.nix -iA 'pkgs.python3Packages.imgaug

@CMCDragonkai
yes, you are correct. I am also facing this hang issue with above command you mentioned.
looks like i had missed something. I will re-inspect and see what is going wrong.

@Rakesh4G
Copy link
Contributor Author

I have updated the error details and other findings on aleju/imgaug#414

@Rakesh4G Rakesh4G force-pushed the imgaug-0.2.9-patch branch 2 times, most recently from 3470632 to 5b52dd5 Compare December 18, 2019 12:32
@Rakesh4G
Copy link
Contributor Author

This PR is ready for merge. All the tests are now passing.
@tbenst @jonringer @CMCDragonkai

jonringer
jonringer approved these changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff LGTM
commit LGTM
tests pass 👍

[3 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/67494
1 package were built:
python37Packages.imgaug

@jonringer
Copy link
Contributor

@GrahamcOfBorg build python37Packages.imgaug

@jonringer jonringer mentioned this pull request Dec 19, 2019
Merged
10 tasks
@jonringer jonringer self-requested a review December 19, 2019 21:17
@CMCDragonkai
Copy link
Member

CMCDragonkai commented Dec 20, 2019 via email

@CMCDragonkai
Copy link
Member

We don't have Mac platforms to test this. So we could forward on the details to imgaug maintainer. While also just only enabling this for Linux?

@CMCDragonkai
Copy link
Member

Another idea is a patch in Nix package level that disables those specific tests.

@jonringer
Copy link
Contributor

personally, I would either do doCheck = stdenv.isLinux or meta.platforms = platforms.linux. The meta platform is probably more appropriate if we don't have some other form of testing.

@Rakesh4G
Copy link
Contributor Author

I have updated the requested changes. Thanks.
@FRidh @jonringer @CMCDragonkai

@jonringer
Copy link
Contributor

@GrahamcOfBorg build python37Packages.imgaug

@tbenst
Copy link
Contributor

tbenst commented Jan 7, 2020

I cherry-picked this patch and has been working for me! would love to see merged

@Rakesh4G Rakesh4G requested a review from FRidh January 7, 2020 05:01
@jonringer jonringer merged commit f412725 into NixOS:master Jan 7, 2020
@Rakesh4G Rakesh4G deleted the imgaug-0.2.9-patch branch January 7, 2020 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbenst tbenst tbenst left review comments

@CMCDragonkai CMCDragonkai Awaiting requested review from CMCDragonkai

@jonringer jonringer Awaiting requested review from jonringer

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
2.status: wait-for-upstream Waiting for upstream fix (or their other action). 6.topic: python 8.has: clean-up 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Rakesh4G @CMCDragonkai @mmahut @jonringer @tbenst @FRidh @veprbl