You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2018/08/07 18:07:24 UTC
[kudu-CR] python: copy pandas dependency into requirements.txt
Hello Jordan Birdsell, Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/11146
to review the following change.
Change subject: python: copy pandas dependency into requirements.txt
......................................................................
python: copy pandas dependency into requirements.txt
This preserves the invariant introduced by commit c93f08393 that if you run
'pip install -r requirements.txt', there won't be any additional downloads
when 'python setup.py test' is run later. The invariant was briefly broken
by commit 997ad765f.
Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
---
M python/requirements.txt
M python/setup.py
2 files changed, 6 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/11146/1
--
To view, visit http://gerrit.cloudera.org:8080/11146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Gerrit-Change-Number: 11146
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jt...@apache.org>
[kudu-CR] python: copy pandas dependency into requirements.txt
Posted by "Jordan Birdsell (Code Review)" <ge...@cloudera.org>.
Jordan Birdsell has posted comments on this change. ( http://gerrit.cloudera.org:8080/11146 )
Change subject: python: copy pandas dependency into requirements.txt
......................................................................
Patch Set 2:
Ok by me. I think the right approach here would be to remove the test requirement from setup.py and add back in the skip test logic + the install of pandas in the jenkins script. This was the original approach but was objected to, but in light of this i think it makes sense to go with the original approach.
--
To view, visit http://gerrit.cloudera.org:8080/11146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Gerrit-Change-Number: 11146
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 08 Aug 2018 04:20:06 +0000
Gerrit-HasComments: No
[kudu-CR] python: copy pandas dependency into requirements.txt
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11146 )
Change subject: python: copy pandas dependency into requirements.txt
......................................................................
python: copy pandas dependency into requirements.txt
This preserves the invariant introduced by commit c93f08393 that if you run
'pip install -r requirements.txt', there won't be any additional downloads
when 'python setup.py test' is run later. The invariant was briefly broken
by commit 997ad765f.
Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Reviewed-on: http://gerrit.cloudera.org:8080/11146
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M python/requirements.txt
M python/setup.py
2 files changed, 6 insertions(+), 0 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Gerrit-Change-Number: 11146
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] python: copy pandas dependency into requirements.txt
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11146 )
Change subject: python: copy pandas dependency into requirements.txt
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Gerrit-Change-Number: 11146
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 07 Aug 2018 23:46:32 +0000
Gerrit-HasComments: No
[kudu-CR] python: copy pandas dependency into requirements.txt
Posted by "Jordan Birdsell (Code Review)" <ge...@cloudera.org>.
Jordan Birdsell has posted comments on this change. ( http://gerrit.cloudera.org:8080/11146 )
Change subject: python: copy pandas dependency into requirements.txt
......................................................................
Patch Set 2: Code-Review-1
I would like to discuss this before it’s committed. Pandas should not be a requirement
--
To view, visit http://gerrit.cloudera.org:8080/11146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Gerrit-Change-Number: 11146
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 08 Aug 2018 00:08:55 +0000
Gerrit-HasComments: No
[kudu-CR] python: copy pandas dependency into requirements.txt
Posted by "Jordan Birdsell (Code Review)" <ge...@cloudera.org>.
Jordan Birdsell has removed a vote on this change.
Change subject: python: copy pandas dependency into requirements.txt
......................................................................
Removed Code-Review-1 by Jordan Birdsell <jt...@apache.org>
--
To view, visit http://gerrit.cloudera.org:8080/11146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Gerrit-Change-Number: 11146
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] python: copy pandas dependency into requirements.txt
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11146 )
Change subject: python: copy pandas dependency into requirements.txt
......................................................................
Patch Set 2:
> I would like to discuss this before it’s committed. Pandas should not be a requirement
Fair, but currently all of our el6-based tests are failing because we lost compatibility. Could we merge this and then discuss better alternatives afterwards?
--
To view, visit http://gerrit.cloudera.org:8080/11146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Gerrit-Change-Number: 11146
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 08 Aug 2018 04:10:56 +0000
Gerrit-HasComments: No
[kudu-CR] python: copy pandas dependency into requirements.txt
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11146 )
Change subject: python: copy pandas dependency into requirements.txt
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11146/1/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/11146/1/python/setup.py@193
PS1, Line 193: Note: dependencies in tests_require should also be listed in
: # requirements.txt so that dependencies aren't downloaded at test-time
: # (when it's more difficult to override various pip installation options).
> Does this also apply to setuptools_scm and unittest2?
I don't know that the opposite of this statement (i.e. that things in requirements.txt should be in tests_require or install_requires) is necessarily true. I've left it alone since I don't fully understand it.
http://gerrit.cloudera.org:8080/#/c/11146/1/python/setup.py@202
PS1, Line 202: cython >= 0.21'
> Not related to this CR, but is it important that this doesn't match up with
Not really, since we guarantee installing Cythin via requirements.txt before this. I guess it could have an impact on deployed systems (i.e. if you run 'pip install kudu-python', pip will satisfy the contents of install_requires first), but I don't know if it really matters or not.
--
To view, visit http://gerrit.cloudera.org:8080/11146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Gerrit-Change-Number: 11146
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 07 Aug 2018 22:24:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] python: copy pandas dependency into requirements.txt
Posted by "Jordan Birdsell (Code Review)" <ge...@cloudera.org>.
Jordan Birdsell has posted comments on this change. ( http://gerrit.cloudera.org:8080/11146 )
Change subject: python: copy pandas dependency into requirements.txt
......................................................................
Patch Set 1:
This was done intentionally. Pandas should be optional, not required.
--
To view, visit http://gerrit.cloudera.org:8080/11146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Gerrit-Change-Number: 11146
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 07 Aug 2018 18:17:20 +0000
Gerrit-HasComments: No
[kudu-CR] python: copy pandas dependency into requirements.txt
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11146 )
Change subject: python: copy pandas dependency into requirements.txt
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11146/1/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/11146/1/python/setup.py@193
PS1, Line 193: Note: dependencies in tests_require should also be listed in
: # requirements.txt so that dependencies aren't downloaded at test-time
: # (when it's more difficult to override various pip installation options).
Does this also apply to setuptools_scm and unittest2?
http://gerrit.cloudera.org:8080/#/c/11146/1/python/setup.py@202
PS1, Line 202: cython >= 0.21'
Not related to this CR, but is it important that this doesn't match up with requirements.txt (cython ==0.26.1)?
--
To view, visit http://gerrit.cloudera.org:8080/11146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadb42535956461fbcb41adc67f5e4c73c310463d
Gerrit-Change-Number: 11146
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 07 Aug 2018 18:17:51 +0000
Gerrit-HasComments: Yes