Skip to content

POC: make core.config self-contained #25176

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

Closed
wants to merge 10 commits into from

Conversation

jbrockmendel
Copy link
Member

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
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

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

Impacted file tree graph

@@            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
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

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.

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

@@ -53,8 +53,13 @@
import re
import warnings

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

Copy link
Contributor

Choose a reason for hiding this comment

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

prob easier to wait till PY2 is out

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I second waiting until PY2 is out.

@jbrockmendel
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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

@topper-123
Copy link
Contributor

@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
Copy link
Contributor

jreback commented Feb 6, 2019

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

@jbrockmendel
Copy link
Member Author

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.

@gfyoung gfyoung added the Refactor Internal refactoring of code label Feb 7, 2019
@jbrockmendel
Copy link
Member Author

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

@jbrockmendel
Copy link
Member Author

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
Copy link
Member Author

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.

@@ -835,3 +837,23 @@ def is_callable(obj):
if not callable(obj):
raise ValueError("Value must be a callable")
return True

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@jbrockmendel
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants