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

MAINT: Post-release deprecations #13040

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

MAINT: Post-release deprecations #13040

wants to merge 10 commits into from

Conversation

larsoner
Copy link
Member

Post-release deprecations, plus some comment rewordings so that after release this line:

clear; git grep [Dd]eprecat -- ':!*.js' ':!*/conftest.py' ':!*/docs.py' ':!*/test_docs.py' mne/

gives way fewer false alarms. I think going forward we should try to reserve variants of the term "deprecation" in our codebase to signal something that we need to change release-to-release as it makes things easier to find. After this PR it only shows:

mne/source_space/_source_space.py:# XXX this should probably be deprecated because it returns surface Labels,
mne/tests/test_docstring_parameters.py:                    and not hasattr(cf, "_deprecated_original")
mne/time_frequency/tfr.py:        layout=None,  # TODO deprecate? not used in orig implementation either
mne/time_frequency/tfr.py:        title=None,  # don't deprecate this one; has (useful) option title="auto"
mne/time_frequency/tfr.py:        title=None,  # TODO consider deprecating this one, or adding an "auto" option
mne/time_frequency/tfr.py:        vmin=None,  # TODO deprecate in favor of `vlim` (needs helper func refactor)
mne/time_frequency/tfr.py:        title=None,  # don't deprecate; topo titles aren't standard (color, size, just.)
mne/time_frequency/tfr.py:        layout=None,  # TODO deprecate; not used in orig implementation
mne/time_frequency/tfr.py:        title=None,  # don't deprecate this one; has (useful) option title="auto"
mne/time_frequency/tfr.py:        vmin=None,  # TODO deprecate in favor of `vlim` (needs helper func refactor)
mne/time_frequency/tfr.py:        title=None,  # don't deprecate; topo titles aren't standard (color, size, just.)
mne/utils/dataframe.py:    # E   DeprecationWarning: The copy keyword is deprecated and will be removed in a

which is actually the set of things we should consider thinking about after a release, like TODO items or in the case of dataframe.py checking if our min pandas version is high enough that we can remove the shim.

We could now consider starting to use the word deprecate to signal other stuff we should look at like other shims that are required for version-by-version changes in other libraries (e.g., typically / most often pandas, matplotlib).

pyproject.toml Outdated
@@ -111,7 +111,7 @@ full-no-qt = [
"nilearn",
"numba",
"openmeeg >= 2.5.5",
"pandas",
"pandas", # >= 1.5
Copy link
Member Author

Choose a reason for hiding this comment

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

Also I noticed in our unification of README and pyproject etc. in #12890 we didn't document anywhere what our internal min (for whichever release is coming up next) is AFAIK. We used to have:

- `pandas <https://pandas.pydata.org>`__ ≥ 1.5.2

Here I've put pandas in there. We could consider explicit pins at some point, but for now comments seem okay. For now I've only added pandas since it's the only one where I removed a shim for < 1.5.

Copy link
Member

Choose a reason for hiding this comment

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

what's the downside to putting an explicit (non-comment) pin right now? Is it that the repo README will reflect pins that don't apply to current stable release? (doesn't seem that bad to me... when we add new deps we add them into pyproject.toml right away in the PR where they're needed so in general I expect the repo README to reflect main not PyPI 🤷🏻)

Copy link
Member Author

Choose a reason for hiding this comment

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

Just that in most cases we are 99% compatible with older versions, and just a single test or two breaks, or some niche plotting function. But it's probably most standard to add an explicit lower bound so I'll do that

@@ -3456,11 +3456,9 @@ def _trigradient(x, y, z):
"""Take gradients of z on a mesh."""
from matplotlib.tri import CubicTriInterpolator, Triangulation

with warnings.catch_warnings(): # catch matplotlib warnings
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also trying some removals of warnings.catch_warnings, we'll see how many of them work 🤞

@drammock
Copy link
Member

drammock commented Dec 19, 2024

I think going forward we should try to reserve variants of the term "deprecation" in our codebase to signal something that we need to change release-to-release as it makes things easier to find.

+1 to this idea/effort. Not sure "Deprecation" is the right signal here. My ad-hoc practice includes comments like

# shim for tfr_array_morlet deprecation warning (TODO: remove after 1.7 release)

which aren't really a deprecation... why not TODO as the signal? or something more specific like RELEASE TODO or TODO v1.9?

EDIT: I realize that the linked comment actually does contain the word "deprecation" but it could easily not have, and still made sense! E.g. something like "shim for matplotlib <2.1" or similar.

@larsoner
Copy link
Member Author

Not sure "Deprecation" is the right signal here. My ad-hoc practice includes comments like ... why not TODO as the signal? or something more specific like RELEASE TODO or TODO v1.9?

Because of this:

$ git grep TODO | wc -l
     111

that's too many to have to go through every release, and most of them are longer-term or things we don't care about so imminently.

RELEASE TODO or something similar would be okay, though.

One advantage of deprecation is that we'll either need to check for that as well / separately (i.e., it becomes a separate step), or make sure any future deprecation-related stuff we add a RELEASE TODO or similar to as a reminder. I guess a separate step isn't so onerous especially since we can probably flag both with a suitable regex (which is what's used in the release Wiki anyway).

@drammock
Copy link
Member

I guess a separate step isn't so onerous especially since we can probably flag both with a suitable regex

never mind my suggestion about RELEASE, it will cause a bunch of false alarms in mne/datasets/config.py and elsewhere. How about this?

git grep -iI "\(deprecat\|futurewarning\)" -- ':!*.js' ':!*/conftest.py' ':!*/docs.py' ':!*/test_docs.py' mne/

that has 62 hits vs 53 from yours, and seems at least slightly better in terms of not missing things that we likely want to change. Whatever you settle on, please add it to tools/dev/Makefile

pyproject.toml Outdated
@@ -111,7 +111,7 @@ full-no-qt = [
"nilearn",
"numba",
"openmeeg >= 2.5.5",
"pandas",
"pandas", # >= 1.5
Copy link
Member

Choose a reason for hiding this comment

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

what's the downside to putting an explicit (non-comment) pin right now? Is it that the repo README will reflect pins that don't apply to current stable release? (doesn't seem that bad to me... when we add new deps we add them into pyproject.toml right away in the PR where they're needed so in general I expect the repo README to reflect main not PyPI 🤷🏻)

Comment on lines +90 to +95
env_file = repo_root / "environment.yml"
old_env = env_file.read_text("utf-8")
if old_env != env:
diff = "\n".join(difflib.unified_diff(old_env.splitlines(), env.splitlines()))
print(f"Updating {env_file} with diff:\n{diff}")
env_file.write_text(env, encoding="utf-8")
Copy link
Member

Choose a reason for hiding this comment

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

❤️

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

Successfully merging this pull request may close these issues.

2 participants