[WIP] DEP: Deprecate is_pathlib_like #14093

hmaarrfk · 2019-07-24T01:13:06Z

This two liner change is part of
#14083

Before: (Master): 105±2ms
After: this branch: 102±1ms

Yes, 3ms, I know, but it adds up.

The only place I found is_pathlike_path is in core/memmap
https://github.com/numpy/numpy/blob/master/numpy/core/memmap.py#L272

numpy/compat/py3k.py

eric-wieser · 2019-07-24T01:29:01Z

Seems worth putting in, especially if we can fold in the cleanup from dropping 2.7 and 3.4

hmaarrfk · 2019-07-24T01:31:36Z

dropping 2.7 and 3.4

Yeah, it is unclear how appropriate it is to do this stuff now, or later during your larger cleanup.

seberg · 2019-07-24T01:38:53Z

Do not hesitate to clean up if its is related to code you touch. Nobody started yet on a serious cleanup, but I think that does not need to stop us to do it in places where it makes life even easier maybe.

hmaarrfk · 2019-07-24T01:48:27Z

Are we OK removing the is_pathlib_path from the compat? It seems like we can do without it.

hmaarrfk · 2019-07-24T01:48:40Z

Or should I just emit a warning.

With the lastest changes:

(owl) mark@mark-xps ~/g/n/numpy avoid_pathlib|+1…1⚑ 1
$ grep is_pathlib_path . -R
./compat/py3k.py:           'integer_types', 'is_pathlib_path', 'npy_load_module',
./compat/py3k.py:def is_pathlib_path(obj):
./compat/py3k.py:    warn("The use of is_pathlib_path is deprecated. "

numpy/compat/py3k.py

doc/release/1.18.0-notes.rst

mattip · 2019-08-15T09:27:15Z

doc/release/1.18.0-notes.rst

+  above, users are encouraged to use ``isinstance(obj, os.PathLike)``. For code
+  supporting Python 3.5, the snippet ``isinstance(obj,
+  numpy.compat.os_PathLike)`` can be used.
+


This should now go into a file in doc/release/upcoming_changes

eric-wieser · 2019-08-25T19:40:00Z

Just two more requests here:

Please add a test to test_deprecations.py
Can you put the deprecation change, test, and release note in its own commit starting DEP instead of MNT? (or even better, its own PR - hiding deprecations in maintenance commits makes it easy for maintainers to not notice them)

hmaarrfk · 2019-08-25T20:38:56Z

So you would like me to make the PR that removes the deprecations in 1.20 today?

rgommers · 2019-08-25T20:40:03Z

@hmaarrfk no, please no PRs that we can't merge for the next year.

hmaarrfk · 2019-08-25T23:26:03Z

Sorry it all makes sense now, you wanted everything contained in a single commit instead of splitting it up into 3.

hmaarrfk · 2019-08-25T23:49:18Z

numpy/compat/py3k.py

+    from pathlib import Path
+    # 2019-08-25, 1.18
+    warn("The use of is_pathlib_path is deprecated. "
+         "For Python 3.5, you should prefer using "


In light of the new NEP, should this warning be revised? Should this whole PR be revised to just make is_pathlib_like the same as os_PathLike???

According to https://github.com/numpy/numpy/blob/master/doc/neps/nep-0029-deprecation_policy.rst#drop-schedule

Should I simply remove references to Python 3.5?

xref gh-14673

eric-wieser · 2019-11-29T09:41:58Z

numpy/compat/py3k.py

-    from pathlib import Path, PurePath
-except ImportError:
-    Path = PurePath = None
+from warnings import warn


This change breaks code that does

from numpy.compat import Path if Path is None: ...

Do we care? This probably at least warrants a release note

yes we probably care. :(

Maybe the only way to cleanup the namespace is to wait until python 3.7 is the only one you support, and then go all out with the deprecations using __getattr__. There is little sense giving warnings to users of python 3.7 + 3.8 only.

sense giving warnings to users of python 3.7 + 3.8 only.

Sure there is, it means that packages using CI across multiple versions can fix their code sooner :)

Anyway, let's leave the slow import alone for now - we can still deprecate the function below.

If nothing else, a change to this file to remove the try / except now that pathlib is always available would be appreciated

There are also ALOT of places that have if/else statements like sys.version > (3, 6), want to remove all of those if else clauses?

lets leave it at pathlib for this PR.

If you remove all the >= (3, 5) (preferably just from this file), then we can merge this in time for 1.18 (which supports 3.5)

whoa, didn't know you all were still going to support 3.5 ;) Not many changes in this case for now. Many if staements can be removed. I'll leave it till then.

hmaarrfk · 2019-11-29T15:43:05Z

numpy/core/tests/test_records.py

@@ -12,7 +12,7 @@
 import pytest

 import numpy as np
-from numpy.compat import Path
+from pathlib import Path


@eric-wieser Do you want to keep changes like this? That modernize the test suite so as not to use our own compatibility layer?

I feel like if we remove these, we are no longer testing the compatibility layer, and it is as good as removing it (since bug will creep in that we won't be monitoring).

If we want to test the compatibility layer, we should have a standalone test for that. I don't think the lack of that test should motivate us not to clean up our other tests.

but as it stands, there are no other tests dedicated for the compatibility layer.

I would prefer to wait unit you drop 3.5 support. then the only test becomes

assert os_PathLike == os.PathLike [...]

eric-wieser · 2019-11-29T17:11:27Z

numpy/core/memmap.py

@@ -269,7 +269,7 @@ def __new__(subtype, filename, dtype=uint8, mode='r+', offset=0,
            self.offset = offset
            self.mode = mode

-            if is_pathlib_path(filename):


This change isn't safe. because it uses resolve below instead of os_fspath - pathlike objects aren't required to implement resolve

So os_PatgLike is a subset of pathlike?

pathlib.Path is a subset of os.PathLike , if that was what you were asking

In [1]: from pathlib import Path In [2]: import os.path In [3]: os.path Out[3]: <module 'posixpath' from '/home/mark/miniconda3/envs/mcam_dev/lib/python3.7/posixpath.py'> In [4]: os.path.abspath(Path()) Out[4]: '/home/mark/git/numpy'

should be ok I think

Apparently only in python 3.8 + https://bugs.python.org/issue9949

You're looking for filename = os_fspath(filename) in this branch - but that becomes a feature, so probably belongs in its own PR

hmaarrfk · 2019-11-29T19:00:01Z

This isn't the "easy removal" I had anticipated. Due to many intricacies in the tests, I think I will have to revise this change from scratch. and maybe keep it to a "cleanup" instead of a "deprecation"

eric-wieser · 2019-11-29T19:01:11Z

numpy/core/memmap.py

@@ -3,7 +3,7 @@
 import numpy as np


Can you backout the changes in this file and save them for a separate, ENH, PR? This is going beyond internal cleanup to (good!) behavior changes.

eric-wieser · 2019-11-29T19:03:07Z

and maybe keep it to a "cleanup" instead of a "deprecation"

That is what I was trying to get at in #14093 (comment) - combining deprecation with cleanup is usually a bad idea, because one requires a lot of thinking to approve, but the latter is often very fast.

mattip · 2020-07-13T05:43:43Z

I am going to close. One of these changes was already merged elsewhere, and others should be split out into another PR. Please feel free to reopen and update.

hmaarrfk force-pushed the avoid_pathlib branch 2 times, most recently from bfce5ae to caf71db Compare July 24, 2019 01:19

eric-wieser reviewed Jul 24, 2019

View reviewed changes

numpy/compat/py3k.py Outdated Show resolved Hide resolved

hmaarrfk force-pushed the avoid_pathlib branch from caf71db to 6c764a2 Compare July 24, 2019 01:37

hmaarrfk force-pushed the avoid_pathlib branch 3 times, most recently from d281983 to 35c5026 Compare July 24, 2019 01:52

hmaarrfk changed the title ~~MNT: Don't import pathlib for python >3.6~~ MNT: Don't import pathlib for python >=3.6 Jul 24, 2019

hmaarrfk mentioned this pull request Jul 24, 2019

WIP, MAINT: Improve import time #14083

Closed

eric-wieser reviewed Jul 24, 2019

View reviewed changes

numpy/compat/py3k.py Outdated Show resolved Hide resolved

eric-wieser reviewed Jul 24, 2019

View reviewed changes

numpy/compat/py3k.py Outdated Show resolved Hide resolved

hmaarrfk force-pushed the avoid_pathlib branch from 35c5026 to b8c581d Compare July 24, 2019 02:45

eric-wieser reviewed Jul 24, 2019

View reviewed changes

doc/release/1.18.0-notes.rst Outdated Show resolved Hide resolved

hmaarrfk force-pushed the avoid_pathlib branch 2 times, most recently from b9e9ab0 to 6e1ea7d Compare July 24, 2019 03:15

charris changed the title ~~MNT: Don't import pathlib for python >=3.6~~ MAINT: Don't import pathlib for python >=3.6 Jul 25, 2019

charris added the 03 - Maintenance label Jul 25, 2019

mattip reviewed Aug 15, 2019

View reviewed changes

hmaarrfk force-pushed the avoid_pathlib branch from 6e1ea7d to fe8a76d Compare August 17, 2019 13:21

hmaarrfk force-pushed the avoid_pathlib branch 2 times, most recently from 318ed3e to aa0d1b1 Compare August 25, 2019 19:14

eric-wieser added the 07 - Deprecation label Aug 25, 2019

hmaarrfk force-pushed the avoid_pathlib branch from aa0d1b1 to 4a5b73c Compare August 25, 2019 23:25

hmaarrfk changed the title ~~MAINT: Don't import pathlib for python >=3.6~~ DEP: Deprecate is_pathlib_like Aug 25, 2019

hmaarrfk commented Aug 25, 2019

View reviewed changes

eric-wieser reviewed Nov 29, 2019

View reviewed changes

hmaarrfk commented Nov 29, 2019

View reviewed changes

DEP: Deprecate the use of is_pathlib_path is favor of os_PathLike

a18b5ea

hmaarrfk force-pushed the avoid_pathlib branch 2 times, most recently from 64b52e3 to a18b5ea Compare November 29, 2019 15:54

Keep PathLike and Path imports

9da7511

hmaarrfk force-pushed the avoid_pathlib branch from d1fab15 to 9da7511 Compare November 29, 2019 16:27

eric-wieser reviewed Nov 29, 2019

View reviewed changes

Remove release note as it is no longer applicable.

0d63c84

hmaarrfk force-pushed the avoid_pathlib branch from be63c96 to 0d63c84 Compare November 29, 2019 18:28

Use abspath instead of resolve

83c25c0

hmaarrfk changed the title ~~DEP: Deprecate is_pathlib_like~~ [WIP] DEP: Deprecate is_pathlib_like Nov 29, 2019

eric-wieser reviewed Nov 29, 2019

View reviewed changes

hmaarrfk mentioned this pull request Nov 29, 2019

MAINT: cleanup compat.py3k.py #15011

Merged

mattip closed this Jul 13, 2020

[WIP] DEP: Deprecate is_pathlib_like #14093

[WIP] DEP: Deprecate is_pathlib_like #14093

Conversation

hmaarrfk commented Jul 24, 2019

eric-wieser commented Jul 24, 2019

hmaarrfk commented Jul 24, 2019

seberg commented Jul 24, 2019

hmaarrfk commented Jul 24, 2019

hmaarrfk commented Jul 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Aug 25, 2019 • edited

hmaarrfk commented Aug 25, 2019

rgommers commented Aug 25, 2019

hmaarrfk commented Aug 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Nov 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Nov 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Nov 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmaarrfk commented Nov 29, 2019

Choose a reason for hiding this comment

eric-wieser commented Nov 29, 2019

mattip commented Jul 13, 2020

hmaarrfk commented Jul 24, 2019 •

edited

eric-wieser commented Aug 25, 2019 •

edited

eric-wieser Nov 29, 2019 •

edited

eric-wieser Nov 29, 2019 •

edited

eric-wieser Nov 29, 2019 •

edited