POC: make core.config self-contained by jbrockmendel · Pull Request #25176 · pandas-dev/pandas

jbrockmendel · 2019-02-06T01:25:35Z

As discussed in #25162. The idea from that issue would then be to move core.config to a location explicitly "upstream" from everything else in pandas.

codecov · 2019-02-06T02:11:06Z

Codecov Report

Merging #25176 into master will decrease coverage by <.01%.
The diff coverage is 88.6%.

@@            Coverage Diff             @@
##           master   #25176      +/-   ##
==========================================
- Coverage   92.37%   92.37%   -0.01%     
==========================================
  Files         166      166              
  Lines       52408    52416       +8     
==========================================
+ Hits        48412    48419       +7     
- Misses       3996     3997       +1

Flag	Coverage Δ
#multiple	`90.79% <88.6%> (-0.01%)`	⬇️
#single	`42.86% <59.49%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/io/formats/printing.py	`85.07% <100%> (-0.35%)`	⬇️
pandas/core/config.py	`86.92% <88.46%> (-0.13%)`	⬇️

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 e3b0950...138c6c2. Read the comment docs.

codecov · 2019-02-06T02:11:08Z

Codecov Report

Merging #25176 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #25176      +/-   ##
==========================================
- Coverage   91.26%   91.26%   -0.01%     
==========================================
  Files         173      173              
  Lines       52968    52970       +2     
==========================================
+ Hits        48340    48341       +1     
- Misses       4628     4629       +1

Flag	Coverage Δ
#multiple	`89.83% <100%> (ø)`	⬆️
#single	`41.71% <57.89%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/config_init.py	`99.21% <ø> (-0.03%)`	⬇️
pandas/core/config.py	`87.29% <100%> (+0.25%)`	⬆️
pandas/util/testing.py	`87.57% <0%> (-0.1%)`	⬇️

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 cc4a7e5...18d471f. Read the comment docs.

jreback

all for the intent. but this is moving printing things into a very weird place.

jreback · 2019-02-06T02:27:48Z

pandas/core/config.py


-import pandas.compat as compat
-from pandas.compat import lmap, map, u
+


prob easier to wait till PY2 is out

Either way works for me. If we decide to move forward with this or something like it, I'd rather get a move on now and clean out the compat code in a few weeks or whenever.

I second waiting until PY2 is out.

…stream

jbrockmendel · 2019-02-06T17:15:16Z

all for the intent. but this is moving printing things into a very weird place.

Yah it does seem like a weird place for pprint_thing. A few thoughts come to mind:

it may be that core.config uses pprint_thing but could get away with something lower-tech. I'll take a look at this.
Part of the idea is that core.config would be moved to somewhere outside of core to make it explicitly "upstream". That might make the scope feel less akward.
pd.io's dependency graph is a mess. In particular, DataFrame, Series, and many of the Index subclasses rely on io.formats.format via circular runtime imports for their __repr__ methods. This isn't a problem, but definitely a bit smelly. This could be seen as a step towards clearing up the pd.io dependency graph.

topper-123 · 2019-02-06T22:16:13Z

FYI, I extracted pd.core.config from pandas a few months ago and made it into self-contained library. It at https://github.com/topper-123/optioneer. You`d be welcome to take a look if it can be used for this.

jbrockmendel · 2019-02-06T23:17:45Z

@topper-123 thanks, thats pretty similar to what I have in mind. Did you find that pprint_thing was unnecessary?

topper-123 · 2019-02-06T23:32:44Z

@jbrockmendel, yes, basically. I wanted to avoid the dependency on pandas or other external dependencies, so I replaced pprint_thing with some custom formatting.

jreback · 2019-02-06T23:40:01Z

wouldn’t be averse to ripping out code and using this

jbrockmendel · 2019-02-07T00:57:22Z

wouldn’t be averse to ripping out code and using this

I haven't looked at it closely enough to have an opinion, but assume that @topper-123 is on the ball. I like the external-library idea in part because it would make it easier to share a config library with the rest of the ecosystem (mentioned in #25162).

Whether internal or external, I think parts of config_init need to go up into it, the display namespace.

…stream

jbrockmendel · 2019-02-07T02:31:18Z

Just using str(...) instead of pprint_thing(...) seems to work fine.

…stream

jbrockmendel · 2019-02-07T05:47:12Z

Updated to move the date_dayfirst and date_yearfirst options from config_init to config. As a result, config could now (if moved outside of core) be imported at the module-level in tslibs.parsing instead of as a runtime import.

w/r/t the PY2 compat, the only compat code this module uses is unicode (or text_type in master), and that is only used for the is_unicode factory, which is never used. We could just remove that object and with it the remaining compat code in this file.

Locally I've been toying with moving config to a pandas._dag directory with __init__-docstring

"""
_dag is an experimental directory for pandas code with:
    - no pandas dependencies
    - self-contained tests

A module or sub-package might be a good fit for _dag if it:
    - is a vendored version of outside code (e.g. tzlocal, Pyperclip)
    - it is needed by _libs and has no pandas dependencies
      (e.g. config, localization)
"""

Thoughts?

jbrockmendel · 2019-02-09T17:18:48Z

After a couple days of trying to wrap my head around the cython codebase, I'm extra-adamant about the benefits of a DAG-as-possible dependency structure for newcomers.

jreback · 2019-02-09T17:34:30Z

pandas/core/config.py

    if not callable(obj):
        raise ValueError("Value must be a callable")
    return True
+


why are you moving this? this defeats the entire purpose of config_init

The goal is to move config out of core (either to another dependency-free directory in pandas or something external like @topper-123's repo) and make it explicitly "upstream" from _libs. That way we get a much cleaner dependency structure by avoiding runtime imports inside _libs.

…stream

jbrockmendel · 2019-03-17T20:29:17Z

Closing in favor of #25613 (and follow-ups)

make core.config self-contained

138c6c2

jreback requested changes Feb 6, 2019

View reviewed changes

jbrockmendel added 2 commits February 6, 2019 08:57

Merge branch 'master' of https://github.com/pandas-dev/pandas into up…

803d4fd

…stream

isort fixup

6f96f3e

jbrockmendel added 2 commits February 6, 2019 16:57

Merge branch 'master' of https://github.com/pandas-dev/pandas into up…

983f3ef

…stream

use low-tech print

022ee4f

gfyoung added the Refactor Internal refactoring of code label Feb 7, 2019

jbrockmendel added 3 commits February 6, 2019 21:19

remove unused PY3

40ca990

Merge branch 'master' of https://github.com/pandas-dev/pandas into up…

33e4b02

…stream

move date opts into config

46216b0

jreback reviewed Feb 9, 2019

View reviewed changes

jbrockmendel added 2 commits February 17, 2019 18:03

Merge branch 'master' of https://github.com/pandas-dev/pandas into up…

25198d0

…stream

Merge branch 'master' of https://github.com/pandas-dev/pandas into up…

18d471f

…stream

jbrockmendel mentioned this pull request Mar 9, 2019

make core.config self-contained #25613

Merged

jbrockmendel closed this Mar 17, 2019

jbrockmendel mentioned this pull request Mar 17, 2019

Move locale code out of tm, into _config #25757

Merged

jbrockmendel deleted the upstream branch September 17, 2019 18:55


		import pandas.compat as compat
		from pandas.compat import lmap, map, u

Uh oh!

Conversation

jbrockmendel commented Feb 6, 2019

Uh oh!

codecov bot commented Feb 6, 2019

Codecov Report

Uh oh!

codecov bot commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

jreback Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

jbrockmendel Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

gfyoung Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

jbrockmendel commented Feb 6, 2019

Uh oh!

topper-123 commented Feb 6, 2019

Uh oh!

jbrockmendel commented Feb 6, 2019

Uh oh!

topper-123 commented Feb 6, 2019

Uh oh!

jreback commented Feb 6, 2019

Uh oh!

jbrockmendel commented Feb 7, 2019

Uh oh!

jbrockmendel commented Feb 7, 2019

Uh oh!

jbrockmendel commented Feb 7, 2019

Uh oh!

jbrockmendel commented Feb 9, 2019

Uh oh!

jreback Feb 9, 2019

Choose a reason for hiding this comment

Uh oh!

jbrockmendel Feb 9, 2019

Choose a reason for hiding this comment

Uh oh!

jbrockmendel commented Mar 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Feb 6, 2019 •

edited

Loading