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

DOC: Bump fastparquet version #24590

Merged
merged 8 commits into from
Jan 5, 2019
Merged

Conversation

TomAugspurger
Copy link
Contributor

Should we just bump the min version to 0.2.1? It's quite new, but I think reading datetime data is somewhat common :) It'll save us bug reports.

@TomAugspurger
Copy link
Contributor Author

cc @jorisvandenbossche this fixes the doc error you were seeing.

@jorisvandenbossche
Copy link
Member

Should we just bump the min version to 0.2.1? It's quite new, but I think reading datetime data is somewhat common :) It'll save us bug reports.

Yeah, maybe that is best ..

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Looks good, let's wait on the doc build to check if it actually worked

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

is this only for docs? may have to update the parquet doc section as well.

@codecov
Copy link

codecov bot commented Jan 3, 2019

Codecov Report

Merging #24590 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24590   +/-   ##
=======================================
  Coverage   92.36%   92.36%           
=======================================
  Files         166      166           
  Lines       52497    52497           
=======================================
  Hits        48489    48489           
  Misses       4008     4008
Flag Coverage Δ
#multiple 90.79% <ø> (ø) ⬆️
#single 43.04% <ø> (ø) ⬆️

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 db051b9...e9f7841. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 3, 2019

Codecov Report

Merging #24590 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24590      +/-   ##
==========================================
+ Coverage   92.37%   92.38%   +<.01%     
==========================================
  Files         166      166              
  Lines       52396    52395       -1     
==========================================
  Hits        48403    48403              
+ Misses       3993     3992       -1
Flag Coverage Δ
#multiple 90.8% <100%> (-0.01%) ⬇️
#single 43% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/io/parquet.py 84.61% <100%> (ø) ⬆️
pandas/core/indexes/period.py 92.06% <0%> (-0.06%) ⬇️
pandas/core/resample.py 97.23% <0%> (ø) ⬆️
pandas/util/testing.py 88.09% <0%> (+0.09%) ⬆️

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 19f715c...d1e0098. Read the comment docs.

@jorisvandenbossche
Copy link
Member

is this only for docs? may have to update the parquet doc section as well.

For now, this PR is only about the doc build. But if the suggestion of Tom is followed to bump the fastparquet version, we should also update the docs (although, if we decide to not nump the minimum version, we should maybe still mention that for datetime data, the latest version is required)

@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

For now, this PR is only about the doc build. But if the suggestion of Tom is followed to bump the fastparquet version, we should also update the docs (although, if we decide to not nump the minimum version, we should maybe still mention that for datetime data, the latest version is required)

then pls do this here

@jreback jreback added Docs IO Parquet parquet, feather labels Jan 3, 2019
@TomAugspurger
Copy link
Contributor Author

I've made the changes to bump the min version to 0.2.1 locally, but I think there's an issue with the conda-forge package. Going to fix that before pushing again.

@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

why are you bumping for parquet reading? (we had already bumped for this release I think).

@TomAugspurger
Copy link
Contributor Author

In the discussion above, Joris and I (and I thought you) agreed that reading datetimes is important enough to bump the minimum required first.

On master, we allow using fastparquet <0.2.1, but that will fail at runtime if trying to read a datetimetz column.

The proposal is to require fastparquet>=0.2.1

@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

its fine. we don't usually bump up this fast, but ok.

@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

but would need to change the tests & docs

@TomAugspurger
Copy link
Contributor Author

#24590 (comment)

but fixing the CF package first.

@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

but fixing the CF package first.

what is this?

@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented Jan 3, 2019 via email

@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

ahh ok

@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

ok merge ok then

@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

merge?

@jorisvandenbossche
Copy link
Member

No , the problem is not yet fixed (see the doc build log on travis)

Possibly due the conda-forge package problem? (@TomAugspurger the conda list output indicates it installed fastparquet 0.2.1 from conda-forge, but then the error is still there and the pd.sho_versions() gives fastparquet 0.1.6 ...)

@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

ok cool.

@TomAugspurger
Copy link
Contributor Author

The fastparquet 0.2.1 conda-forge package was accidentally packaging 0.1.6 (but labeling it as 0.2.1), so the error persisted.

The linux builds for a corrected fastparquet are just finishing now. I'll clean up my version bump and push.

@jreback jreback added this to the 0.24.0 milestone Jan 4, 2019
ci/deps/azure-macos-35.yaml Show resolved Hide resolved
@jorisvandenbossche
Copy link
Member

Some travis builds are failing (didn't check why), but just checked that the doc build is now working (the error is gone)

@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented Jan 4, 2019

The travis build is failing because not all the conda packages are done building. Some are still pulling in the (bad) 0.2.1 fastparquet. Conda install the bad 0.2.1, but then fastparquet.__version__ is 0.1.6 (so we know that code works at least).

I'll let those finish up before pushing again (with the dev env update).

@TomAugspurger
Copy link
Contributor Author

FYI, conda-forge stopped building packages for Python 3.5 on MacOS, so I've removed fastparquet from that env.

@jreback jreback merged commit 8506e66 into pandas-dev:master Jan 5, 2019
@jreback
Copy link
Contributor

jreback commented Jan 5, 2019

thanks!

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
@TomAugspurger TomAugspurger deleted the doc-fastparquet branch November 21, 2019 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs IO Parquet parquet, feather
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants