[MRG] Auto generates deprecation for sklearn.utils.mocking #15071

thomasjpfan · 2019-09-23T20:21:17Z

Reference Issues/PRs

Alternative to #15039

What does this implement/fix? Explain your changes.

Autogenerates deprecation file to preserve git glame.

CC @NicolasHug

NicolasHug

If that also works when changes are added to _mocking then I'm happy with it

NicolasHug · 2019-09-23T20:39:10Z

sklearn/setup.py

+
+
+# Adds files that will be deprecated
+def _adds_deprecated_submodules():


Suggested change

def _adds_deprecated_submodules():

def _add_deprecated_submodules():

Also it would need a short description (remaning files + creating new file to keep the import)

NicolasHug · 2019-09-23T20:39:10Z

sklearn/setup.py

 import os

 from sklearn._build_utils import maybe_cythonize_extensions


+_DEPRECATED_MODULES = {


Maybe put this whole logic in another file so we can just mark it to be removed in 0.24

.gitignore

jnothman · 2019-09-23T21:24:26Z

Will this work at conclusion?

thomasjpfan · 2019-09-23T21:37:48Z

At conclusion?

jnothman · 2019-09-23T22:00:27Z

At conclusion of the deprecation, i.e. 0.24

thomasjpfan · 2019-09-23T22:18:06Z

At conclusion, the code to generate the file will be removed. In this case, mocking.py will not be generated anymore.

jnothman · 2019-09-23T22:21:02Z

Of course. So the rename here preserved history as long as no file is created with the old name.

NicolasHug · 2019-09-23T22:32:35Z

The file is created, it's just git ignored. That's the main difference with the other strategy

thomasjpfan · 2019-09-23T23:05:07Z

The file is not in the repo. It is only created during setup.

NicolasHug · 2019-09-24T13:07:39Z

@thomasjpfan can you check that this doesn't create complex merge conflict when:

changes are made to _file.py
the whole thing is squashed and merged into master

thomasjpfan · 2019-09-24T14:39:19Z

@NicolasHug As a demo, I used master_test_auto_gen to simulate master on my fork:

Created a PR thomasjpfan#13 that merges and squashes this PR into master_test_auto_gen.
Create a PR that wants to merge into master_test_auto_gen: thomasjpfan#14

Looks like there are no merge conflicts.

NicolasHug · 2019-09-24T16:40:23Z

It looks like the thomasjpfan#14 PR is directly modifying _mocking.py.

We should instead try to make changes to mocking.py from a branch that starts from master before thomasjpfan:auto_generate_deprecation

thomasjpfan · 2019-09-24T17:19:49Z

thomasjpfan#15 there is a merge conflict. Running git merge master_test_auto_gen was able to automatically rename the file and resolve the conflict.

NicolasHug · 2019-09-24T17:56:23Z

Having tested this locally it seems that it does create merge conflicts, but they are really trivial to solve now (since the file.py isn't tracked by git anymore.)

I'm +1

NicolasHug · 2019-09-24T18:00:53Z

ping @adrinjalali @glemaitre @rth I think this would enable the large scale deprecations that we've been trying

…recation

NicolasHug

Nits about names but LGMT

NicolasHug · 2019-09-25T18:50:31Z

sklearn/_build_utils/deprecated_modules.py

+
+
+_DEPRECATED_MODULES = {
+    # TODO: Remove in 0.24


The entire file should be removed in 0.24 right? (ideally)

That is the ideal case. ;)

NicolasHug · 2019-09-25T18:50:31Z

sklearn/_build_utils/deprecated_modules.py

+"""
+
+
+def _add_deprecated_submodules():


Suggested change

def _add_deprecated_submodules():

def _create_deprecated_modules_files():

?

NicolasHug · 2019-09-25T18:50:31Z

sklearn/_build_utils/deprecated_modules.py

+    ('_mocking', 'sklearn.utils.mocking', 'sklearn.utils')
+}
+
+_DEPRECATE_TEMPLATE = """from .{module} import *  # noqa


Suggested change

_DEPRECATE_TEMPLATE = """from .{module} import * # noqa

_FILE_CONTENT_TEMPLATE = """from .{module} import * # noqa

NicolasHug · 2019-09-25T18:50:31Z

sklearn/_build_utils/deprecated_modules.py

+
+
+_DEPRECATED_MODULES = {
+    # TODO: Remove in 0.24


Please add comment indicating that the 3tuple is (underscored_filed, deprecated_path, correct_path)

NicolasHug · 2019-09-25T19:29:38Z

CLN Addresses @NicolasHug comments

Don't ping me on the commit comments please ^^

NicolasHug · 2019-09-25T19:32:07Z

sklearn/_build_utils/deprecated_modules.py

+from pathlib import Path
+
+# This is a set of 3-tuples consisting of
+# (new_module_path, deprecated_path, correct_path)


Suggested change

# (new_module_path, deprecated_path, correct_path)

# (new_module_name, deprecated_path, correct_import_path)

thomasjpfan · 2019-09-26T04:34:33Z

I didn’t notice that it pings. I’ll remove the @ next time.

adrinjalali

I think we should also remove those files in make clean, do we?

adrinjalali · 2019-09-26T17:20:30Z

sklearn/_build_utils/deprecated_modules.py

+        deprecated_parts = deprecated_path.split(".")
+        deprecated_parts[-1] = deprecated_parts[-1] + ".py"
+
+        with Path(*deprecated_parts).open('w') as f:


we don't mind potentially overriding those files?

I do not see any side effect of this. This can be good if we decide to adjust the template?

NicolasHug · 2019-09-27T14:08:11Z

Was wondering, are the files gonna be shipped with the wheels??

thomasjpfan · 2019-09-27T18:30:46Z

Currently, I think it is shipped with the wheels. Is that an issue?

NicolasHug · 2019-09-27T18:37:42Z

It's an issue if they're not

thomasjpfan · 2019-09-27T19:01:20Z

Confirmed it is in the wheel.

adrinjalali

LGTM, thanks @thomasjpfan

jnothman

Please add to what's new under sklearn.utils

NicolasHug · 2019-10-02T14:16:41Z

Thanks @thomasjpfan

@adrinjalali @glemaitre let's update our PRs :)

glemaitre · 2019-10-23T10:06:13Z

Just getting back to these PRs. This solution looks nice :)

NicolasHug and others added 6 commits Sep 20, 2019

git mved mocking to _mocking

e70b29e

Add new mocking.py file + added tests

4f4f0ab

fixed import to use underscore

2712017

ENH Auto generates deprecation

cdce9d3

ENH Remove mocking

763ce3a

BUG Change add submodule order

93c3304

thomasjpfan changed the title ~~[MRG] Auto generates deprecation~~ [MRG] Auto generates deprecation for sklearn.utils.mocking Sep 23, 2019

thomasjpfan added 2 commits Sep 23, 2019

BUG Fix path

35848f0

DOC Adds comment

df9684b

NicolasHug reviewed Sep 23, 2019

View changes

CLN Addresses @NicolasHug comments

1bdb28a

thomasjpfan added 2 commits Sep 24, 2019

ENH Computes relative path

154b8cd

Merge remote-tracking branch 'upstream/master' into auto_generate_dep…

1e3d538

…recation

NicolasHug approved these changes Sep 25, 2019

View changes

CLN Addresses @NicolasHug comments

fb2189b

NicolasHug reviewed Sep 25, 2019

View changes

adrinjalali reviewed Sep 26, 2019

View changes

ENH Updates Makefile to clean generated files

5f18687

adrinjalali approved these changes Oct 2, 2019

View changes

jnothman reviewed Oct 2, 2019

View changes

thomasjpfan and others added 2 commits Oct 2, 2019

DOC Added deprecated mocking classes to whats new

af5e7e9

Merge branch 'master' into auto_generate_deprecation

2874d25

jnothman merged commit dba753a into scikit-learn:master Oct 2, 2019
3 of 13 checks passed

NicolasHug mentioned this pull request Oct 2, 2019

[MRG] MNT Rename utils/mocking into utils/_mocking #15039

Closed

scikit-learn / scikit-learn Public

[MRG] Auto generates deprecation for sklearn.utils.mocking #15071

[MRG] Auto generates deprecation for sklearn.utils.mocking #15071

thomasjpfan commented Sep 23, 2019

NicolasHug left a comment •

edited

NicolasHug Sep 23, 2019

NicolasHug Sep 23, 2019

jnothman commented Sep 23, 2019

thomasjpfan commented Sep 23, 2019

jnothman commented Sep 23, 2019

thomasjpfan commented Sep 23, 2019

jnothman commented Sep 23, 2019

NicolasHug commented Sep 23, 2019

thomasjpfan commented Sep 23, 2019

NicolasHug commented Sep 24, 2019

thomasjpfan commented Sep 24, 2019

NicolasHug commented Sep 24, 2019

thomasjpfan commented Sep 24, 2019

NicolasHug commented Sep 24, 2019

NicolasHug commented Sep 24, 2019

NicolasHug left a comment

NicolasHug Sep 25, 2019

thomasjpfan Sep 25, 2019

NicolasHug Sep 25, 2019

NicolasHug Sep 25, 2019

NicolasHug Sep 25, 2019

NicolasHug commented Sep 25, 2019

NicolasHug Sep 25, 2019

thomasjpfan commented Sep 26, 2019

adrinjalali left a comment

adrinjalali Sep 26, 2019

thomasjpfan Sep 27, 2019

NicolasHug commented Sep 27, 2019

thomasjpfan commented Sep 27, 2019 •

edited

NicolasHug commented Sep 27, 2019

thomasjpfan commented Sep 27, 2019

adrinjalali left a comment

jnothman left a comment

NicolasHug commented Oct 2, 2019

glemaitre commented Oct 23, 2019



		# Adds files that will be deprecated
		def _adds_deprecated_submodules():

	def _adds_deprecated_submodules():
	def _add_deprecated_submodules():

	def _add_deprecated_submodules():
	def _create_deprecated_modules_files():

	_DEPRECATE_TEMPLATE = """from .{module} import * # noqa
	_FILE_CONTENT_TEMPLATE = """from .{module} import * # noqa

	# (new_module_path, deprecated_path, correct_path)
	# (new_module_name, deprecated_path, correct_import_path)

scikit-learn / scikit-learn Public

[MRG] Auto generates deprecation for sklearn.utils.mocking #15071

[MRG] Auto generates deprecation for sklearn.utils.mocking #15071

Conversation

thomasjpfan commented Sep 23, 2019

Reference Issues/PRs

What does this implement/fix? Explain your changes.

NicolasHug left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Sep 23, 2019

thomasjpfan commented Sep 23, 2019

jnothman commented Sep 23, 2019

thomasjpfan commented Sep 23, 2019

jnothman commented Sep 23, 2019

NicolasHug commented Sep 23, 2019

thomasjpfan commented Sep 23, 2019

NicolasHug commented Sep 24, 2019

thomasjpfan commented Sep 24, 2019

NicolasHug commented Sep 24, 2019

thomasjpfan commented Sep 24, 2019

NicolasHug commented Sep 24, 2019

NicolasHug commented Sep 24, 2019

NicolasHug left a 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

NicolasHug commented Sep 25, 2019

Choose a reason for hiding this comment

thomasjpfan commented Sep 26, 2019

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasHug commented Sep 27, 2019

thomasjpfan commented Sep 27, 2019 • edited

NicolasHug commented Sep 27, 2019

thomasjpfan commented Sep 27, 2019

adrinjalali left a comment

jnothman left a comment

NicolasHug commented Oct 2, 2019

glemaitre commented Oct 23, 2019

NicolasHug left a comment •

edited

thomasjpfan commented Sep 27, 2019 •

edited