Skip to content
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

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

Merged
merged 15 commits into from Oct 2, 2019

Conversation

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Sep 23, 2019

Reference Issues/PRs

Alternative to #15039

What does this implement/fix? Explain your changes.

Autogenerates deprecation file to preserve git glame.

CC @NicolasHug

@thomasjpfan thomasjpfan changed the title [MRG] Auto generates deprecation [MRG] Auto generates deprecation for sklearn.utils.mocking Sep 23, 2019
Copy link
Member

@NicolasHug NicolasHug left a comment

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



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

This comment has been minimized.

@NicolasHug

NicolasHug Sep 23, 2019
Member

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)

import os

from sklearn._build_utils import maybe_cythonize_extensions


_DEPRECATED_MODULES = {

This comment has been minimized.

@NicolasHug

NicolasHug Sep 23, 2019
Member

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

.gitignore Show resolved Hide resolved
@jnothman
Copy link
Member

@jnothman jnothman commented Sep 23, 2019

@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Sep 23, 2019

At conclusion?

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 23, 2019

@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Sep 23, 2019

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

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 23, 2019

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Sep 23, 2019

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

@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Sep 23, 2019

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

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Sep 24, 2019

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

@thomasjpfan thomasjpfan commented Sep 24, 2019

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

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

Looks like there are no merge conflicts.

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Sep 24, 2019

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

@thomasjpfan thomasjpfan commented Sep 24, 2019

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

@NicolasHug NicolasHug commented Sep 24, 2019

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

@NicolasHug NicolasHug commented Sep 24, 2019

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

Copy link
Member

@NicolasHug NicolasHug left a comment

Nits about names but LGMT



_DEPRECATED_MODULES = {
# TODO: Remove in 0.24

This comment has been minimized.

@NicolasHug

NicolasHug Sep 25, 2019
Member

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

This comment has been minimized.

@thomasjpfan

thomasjpfan Sep 25, 2019
Author Member

That is the ideal case. ;)

"""


def _add_deprecated_submodules():

This comment has been minimized.

@NicolasHug

NicolasHug Sep 25, 2019
Member

Suggested change
def _add_deprecated_submodules():
def _create_deprecated_modules_files():

?

('_mocking', 'sklearn.utils.mocking', 'sklearn.utils')
}

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

This comment has been minimized.

@NicolasHug

NicolasHug Sep 25, 2019
Member

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


_DEPRECATED_MODULES = {
# TODO: Remove in 0.24

This comment has been minimized.

@NicolasHug

NicolasHug Sep 25, 2019
Member

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

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Sep 25, 2019

CLN Addresses @NicolasHug comments

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

from pathlib import Path

# This is a set of 3-tuples consisting of
# (new_module_path, deprecated_path, correct_path)

This comment has been minimized.

@NicolasHug

NicolasHug Sep 25, 2019
Member

Suggested change
# (new_module_path, deprecated_path, correct_path)
# (new_module_name, deprecated_path, correct_import_path)
@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Sep 26, 2019

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

Copy link
Member

@adrinjalali adrinjalali left a comment

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

deprecated_parts = deprecated_path.split(".")
deprecated_parts[-1] = deprecated_parts[-1] + ".py"

with Path(*deprecated_parts).open('w') as f:

This comment has been minimized.

@adrinjalali

adrinjalali Sep 26, 2019
Member

we don't mind potentially overriding those files?

This comment has been minimized.

@thomasjpfan

thomasjpfan Sep 27, 2019
Author Member

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

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Sep 27, 2019

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

@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Sep 27, 2019

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

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Sep 27, 2019

It's an issue if they're not

@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Sep 27, 2019

Confirmed it is in the wheel.

Copy link
Member

@adrinjalali adrinjalali left a comment

LGTM, thanks @thomasjpfan

Copy link
Member

@jnothman jnothman left a comment

Please add to what's new under sklearn.utils

@jnothman jnothman merged commit dba753a into scikit-learn:master Oct 2, 2019
3 of 13 checks passed
3 of 13 checks passed
@azure-pipelines
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas failed
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py35_ubuntu_atlas) Linux py35_ubuntu_atlas failed
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux pylatest_conda_mkl) Linux pylatest_conda_mkl failed
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas failed
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux32 py35_ubuntu_atlas_32bit) Linux32 py35_ubuntu_atlas_32bit failed
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py35_pip_openblas_32bit) Windows py35_pip_openblas_32bit failed
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl failed
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_mkl) macOS pylatest_conda_mkl failed
Details
@azure-pipelines
scikit-learn.scikit-learn
Details
ci/circleci: lint CircleCI is running your tests
Details
@lgtm-com
LGTM analysis: JavaScript No code changes detected
Details
@lgtm-com
LGTM analysis: C/C++ No new or fixed alerts
Details
@lgtm-com
LGTM analysis: Python No new or fixed alerts
Details
@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Oct 2, 2019

Thanks @thomasjpfan

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

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Oct 23, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants