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

BLD: minor break ci/requirements-optional-pip.txt #22889

Merged
merged 4 commits into from
Sep 30, 2018

Conversation

minggli
Copy link
Contributor

@minggli minggli commented Sep 29, 2018

When adding optional dependencies, below error appears from

Invalid requirement: 'openpyxl=2.5.5'
= is not a valid operator. Did you mean == ?

To core dev, should we fix the version or simply specify lower bound (i.e. >=)?

@minggli minggli changed the title BLD: minor break ci/requirements-optional-pip.txt BLD: minor break ci/requirements-optional-*.txt Sep 29, 2018
@datapythonista
Copy link
Member

Thanks for the PR @minggli. Those files are autogenerated. Can you check the comments in this PR: #22749 and fix in the source file please (and then autogenerate the ones you changed)?

@datapythonista datapythonista added Build Library building on various platforms Dependencies Required and optional dependencies labels Sep 29, 2018
@datapythonista
Copy link
Member

I think we want that specific version of openpyxl, so it'd be openpyxl=2.5.5 (not >=). Can you confirm @TomAugspurger

@codecov
Copy link

codecov bot commented Sep 29, 2018

Codecov Report

Merging #22889 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22889      +/-   ##
==========================================
+ Coverage   92.18%   92.21%   +0.02%     
==========================================
  Files         169      169              
  Lines       50830    50975     +145     
==========================================
+ Hits        46860    47006     +146     
+ Misses       3970     3969       -1
Flag Coverage Δ
#multiple 90.63% <ø> (+0.02%) ⬆️
#single 42.42% <ø> (+0.05%) ⬆️
Impacted Files Coverage Δ
pandas/core/frame.py 97.44% <0%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e45a6c1...57ec327. Read the comment docs.

@TomAugspurger
Copy link
Contributor

Correct. We need to update scripts/convert_deps.py to rewrite exact pins as == for pip.

@minggli
Copy link
Contributor Author

minggli commented Sep 29, 2018

Thanks for the PR @minggli. Those files are autogenerated. Can you check the comments in this PR: #22749 and fix in the source file please (and then autogenerate the ones you changed)?

Thanks @datapythonista for looking into it. The script/convert_deps.py uses requirements-optional-conda.txt as base to generate, which has been fixed already.

with open("ci/requirements-optional-conda.txt") as f:
    optional = [x.strip() for x in f.readlines()]

so this should conclude the PR.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Cool, I didn't remember that. Lgtm. Thanks for the fix @minggli

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Sorry, I think I misunderstood. If I understand correctly, the conda file required one equal only (so you shouldn't add the extra one), and the script convert_deps.py should be updated to make it two in the pip file. And after that generate the pip file.

@pep8speaks
Copy link

Hello @minggli! Thanks for updating the PR.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm

It can surely be in a separate PR, but I think at some point it'd be nice to have a single function in the script that we call twice (one for required and one for optional), and that we unit test.

@minggli
Copy link
Contributor Author

minggli commented Sep 29, 2018

good point. thanks Marc.

@minggli minggli changed the title BLD: minor break ci/requirements-optional-*.txt BLD: minor break ci/requirements-optional-pip.txt Sep 30, 2018
@minggli minggli closed this Sep 30, 2018
@minggli minggli reopened this Sep 30, 2018
@minggli minggli closed this Sep 30, 2018
@minggli minggli reopened this Sep 30, 2018
@jreback jreback added this to the 0.24.0 milestone Sep 30, 2018
@jreback jreback merged commit f4fae35 into pandas-dev:master Sep 30, 2018
@jreback
Copy link
Contributor

jreback commented Sep 30, 2018

thanks. this is a dev requirement only.

@minggli
Copy link
Contributor Author

minggli commented Sep 30, 2018

thanks for reviewing @jreback @datapythonista @TomAugspurger!

@minggli minggli deleted the docs/requirements branch September 30, 2018 14:08
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Library building on various platforms Dependencies Required and optional dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KeyError on master in TestExcelWrtiter
5 participants