TYP: Adding absolute imports for pandas.core.reshape #25936
Conversation
|
Removed concat and tile from mypy.ini. I've not made any changes to imports for merge module due to issue mentioned in #25934 regarding pandas._libs |
|
Is that failing for you locally? Curious if you are able to investigate root cause |
Codecov Report
@@ Coverage Diff @@
## master #25936 +/- ##
===========================================
- Coverage 91.81% 41.91% -49.91%
===========================================
Files 175 175
Lines 52580 52592 +12
===========================================
- Hits 48278 22044 -26234
- Misses 4302 30548 +26246
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25936 +/- ##
==========================================
- Coverage 91.9% 91.89% -0.01%
==========================================
Files 175 175
Lines 52485 52485
==========================================
- Hits 48234 48230 -4
- Misses 4251 4255 +4
Continue to review full report at Codecov.
|
No I didn't run any test locally - i can try and report back if that helps |
|
Just tried running tests locally with importing |
pandas/core/reshape/tile.py
Outdated
| import pandas.core.algorithms as algos | ||
| from pandas.core.arrays.categorical import Categorical |
There was a problem hiding this comment.
@WillAyd this is going to make
imports much longer
no way to simply import from pandas
directly?
There was a problem hiding this comment.
When you say longer you just mean more lines right? I thought this was desired from conversation in #25923
There was a problem hiding this comment.
i was thinking just for frame / series but now i see for everything
this is pretty jarring - is there anyway to do this w/o going all the way here?
There was a problem hiding this comment.
So here's a relevant issue I found on the MyPy tracker:
I think there's a few ways to go about this which aren't mutually exclusive:
- Go with path above of explicitly import objects where they are defined internally
- Replace wildcard imports in top level init with actual objects
- Wait for new mypy semantic analyzer (note I tried current dev version and it raised AssertionError, so not sure how far out this would be or if viable)
- Stub out top level pandas objects
I think we want number 4 at the end of the day and ultimately would want to put something like that in TypeShed, but my thought was that is still going to take some effort to generate correctly and it would be good to get our blacklist of items whittled down first before focusing on that.
While number 1 above is more verbose I think it has the added bonus of making our internal import machinery more explicit, so I'd still vouch for it here
There was a problem hiding this comment.
we should just do this: Replace wildcard imports in top level init with actual objects, this is much better that changing everything else. @WillAyd can you make an issue for this (though you have one about changing to absolute imports everywhere)
|
Gotcha. Yea I’m sure there’s another way then just need to learn more about the import machinery
…Sent from my iPhone
On Mar 31, 2019, at 9:00 AM, Jeff Reback ***@***.***> wrote:
@jreback commented on this pull request.
In pandas/core/reshape/tile.py:
> import pandas.core.algorithms as algos
+from pandas.core.arrays.categorical import Categorical
i was thinking just for frame / series but now i see for everything
this is pretty jarring - is there anyway to do this w/o going all the way here?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
There was a problem hiding this comment.
@WillAyd these are two other places which required top level imports after reverting all changes in this PR. Ive added them in latest commits
pandas/core/reshape/concat.py
Outdated
| @@ -2,20 +2,20 @@ | |||
| concat routines | |||
| """ | |||
|
|
|||
| import numpy as np | |||
|
|
|||
| import pandas.core.dtypes.concat as _concat | |||
pandas/core/reshape/tile.py
Outdated
| from pandas._libs.lib import infer_dtype | ||
|
|
||
| from pandas.core.dtypes.common import ( | ||
| _NS_DTYPE, ensure_int64, is_categorical_dtype, is_datetime64_dtype, | ||
| is_datetime64tz_dtype, is_datetime_or_timedelta_dtype, is_integer, | ||
| is_scalar, is_timedelta64_dtype) | ||
| from pandas.core.dtypes.missing import isna |
|
Similar comment in the other issue - right now keeping focus on typing so if it works by just removing blacklist items let's stick to that. Certainly don't want to import numpy from the pandas namespace |
|
ok just reverted the changes now |
pandas/core/reshape/concat.py
Outdated
|
|
||
| from pandas import DataFrame, Index, MultiIndex, Series, compat | ||
| from pandas import ( | ||
| DataFrame, Index, MultiIndex, Series, compat, concat as _concat) |
There was a problem hiding this comment.
Looks like this is causing a build failure
ryankarlos
force-pushed
the
Typing_fixups_pandas.core.reshape
branch
from
17f8172 to
7755c8e
Compare
ryankarlos
force-pushed
the
Typing_fixups_pandas.core.reshape
branch
from
7755c8e to
ad8ca67
Compare
|
@ryankarlos looks OK and doubt CI failures are related but can you merge master and repush? FYI merging master is strongly preferred to rebasing and shouldn't require a force push. Your workflow should look something like this on your local branch: git fetch upstream
git merge upstream master
# Fix up merge conflicts if any
# git merge --continue
git push origin YOUR_BRANCH |
|
Test failures were unrelated and disappeared on restart of CI. This is pretty stripped down from original request and looks good so moving forward with merge. Thanks @ryankarlos |
ryankarlos
changed the title
ryankarlos
changed the title
git diff upstream/master -u -- "*.py" | flake8 --diff