MRG: Add return type hints to `read_evokeds()`, `read_raw()` #12250

hoechenberger · 2023-11-30T17:44:48Z

This PR adds return type hints for read_evokeds() and read_raw().

I decided to annotate read_raw() as BaseRaw, as this is the parent class shared among all raw readers.

Neat:
Turns out that since all specific raw readers (read_raw_*()) are structured in a simple fashion and have return RawXXX() as their last line of code, VS Code can trivially find out the correct return types of those functions without any additional type hints!

Same goes for read_epochs(), hence no changes are required there either.

Still, our docstrings are, strictly speaking, incorrect; read_raw_X() returns a RawX instance, not Raw; and read_epochs() returns EpochsFIF, not Epochs. Just something to keep in mind for a future cleanup or so.

larsoner · 2023-12-01T00:55:48Z

Looks good to me so far. Is it reasonable to add mypy to check just these functions, or no? If so I can try adding it to this PR. And @hoechenberger we probably need a doc/changes/devel.rst update mentioning that some return type hints have been added.

hoechenberger · 2023-12-01T05:45:04Z

I will look into this, @larsoner

hoechenberger · 2023-12-01T13:04:04Z

@larsoner I added the foundations for MyPy testing, but I don't have time to adjust the CI configurations etc

❯ mypy ./mne
Success: no issues found in 638 source files

Changing the read_evokeds() return type from Union[List[Evoked], Evoked] to List[Evoked]:

❯ mypy ./mne
mne/evoked.py:1630: error: Incompatible return value type (got "object", expected "list[Evoked]")  [return-value]
Found 1 error in 1 file (checked 638 source files)

Not the greatest error message, but it does discover the bug!

There's an issue with BunchConstNamed, which has occurred to me for years but so far I managed to silently ignore it; but with MyPy, this is no longer an option and I had to explicitly # type: ignore lines where we're accessing FIFF.*, otherwise we'd get messages like:

mne/evoked.py:83: error: "BunchConstNamed" has no attribute "FIFFV_ASPECT_AVERAGE"  [attr-defined]

I've gotten red squiggly lines and no tab completion with BunchConstNamed ever since I started using VS Code, so something is not quite right there. Not sure how to fix it properly. I wonder if we should simply switch to SimpleNamespace?

hoechenberger · 2023-12-01T13:09:12Z

I will write a changeog entry later

larsoner · 2023-12-04T20:02:04Z

I don't have time to adjust the CI configurations etc... I've gotten red squiggly lines and no tab completion with BunchConstNamed ever since I started using VS Code, so something is not quite right there. Not sure how to fix it properly. I wonder if we should simply switch to SimpleNamespace

Maybe but for now I just ignore attr-defined, we hit it on other stuff, too, so it'll be a bigger undertaking. Now added to CIs via pre-commit.ci and it's passing, as is pre-commit run --all locally and mypy mne/ locally.

Based on the link @drammock posted in #12249, I think we're on step https://mypy.readthedocs.io/en/stable/existing_code.html#prioritise-annotating-widely-imported-modules at this point, so @hoechenberger if you add a devel.rst update we should be good to go here I think.

pyproject.toml

hoechenberger · 2023-12-05T14:41:27Z

pyproject.toml

+    'no-untyped-call',
+    'no-untyped-def',
+    'misc',
+    'assignment',
+    'operator',


@larsoner I don't need to add a single one of these to successfully run mypy mne/ without errors. Did you only add those in anticipation of future errors? I'd rather remove or comment out anything we don't need right now. I'm using MyPy 1.7.1.

Ah, you were running it with --strict. Why?

I don't think we should run with --strict for now.

MNE is basically not typed at all; telling MyPy to be strict is just setting us up for all kinds of trouble

Also the MyPy docs say:

Note: the exact list of flags enabled by running --strict may change over time.

To me it's better to know and be explicit here but we can remove it if you want

(I'd like to keep the comments though because they do indicate things we can work on in the future)

* upstream/main: MRG: Use ruff-format instead of Black (mne-tools#12261) MAINT: Make selenium optional and use on CircleCI (mne-tools#12263) MAINT: Post-release deprecations (mne-tools#12265) MAINT: Replace `Path.parent.parent` with `Path.parents[N]` in tests (mne-tools#12257)

larsoner · 2023-12-05T16:50:45Z

@hoechenberger fixed a conflict and added to .git-blame-revs-ignore from the other PR, good to go from your end?

larsoner

@drammock feel free to review and approve then we can merge I think assuming @hoechenberger is also happy

drammock · 2023-12-05T20:15:47Z

I do see the type:

but "loading" stays there for tens of seconds. Ctrl+space after raw. pops up a bigger box with "loading..." in a bigger font.

hoechenberger · 2023-12-05T20:41:18Z

This looks like a problem with Pylance. Is there anything useful logged in the "Python language server" log window?

drammock · 2023-12-05T21:11:13Z

nothing useful to my eye:

2023-12-05 15:08:06.092 [info] [Info  - 3:08:06 PM] (2507452) Pylance language server 2023.11.10 (pyright 088ebaa5) starting
2023-12-05 15:08:06.092 [info] [Info  - 3:08:06 PM] (2507452) Server root directory: /home/drmccloy/.vscode/extensions/ms-python.vscode-pylance-2023.11.10/dist
2023-12-05 15:08:06.104 [info] [Info  - 3:08:06 PM] (2507452) Starting service instance "python"
2023-12-05 15:08:06.143 [info] [Info  - 3:08:06 PM] (2507452) Setting pythonPath for service "python": "/opt/mambaforge/envs/mnedev/bin/python"
2023-12-05 15:08:06.144 [info] [Info  - 3:08:06 PM] (2507452) Setting environmentName for service "python": "3.11.6 (mnedev conda)"
2023-12-05 15:08:06.147 [info] [Info  - 3:08:06 PM] (2507452) Loading pyproject.toml file at /opt/mne/python/pyproject.toml
2023-12-05 15:08:06.163 [info] [Info  - 3:08:06 PM] (2507452) Pyproject file "/opt/mne/python/pyproject.toml" has no "[tool.pyright]" section.
2023-12-05 15:08:06.214 [info] [Info  - 3:08:06 PM] (2507452) Assuming Python version 3.11
2023-12-05 15:08:06.620 [info] [Info  - 3:08:06 PM] (2507452) Found 1845 source files
2023-12-05 15:08:17.420 [info] [Info  - 3:08:17 PM] (2507452) [IDX(2)] Long operation: index execution environment /opt/mne/python (6534ms)
2023-12-05 15:08:17.421 [info] [Info  - 3:08:17 PM] (2507452) [IDX(2)] Long operation: index packages /opt/mne/python (6561ms)

all of this was up within ~15 seconds of opening vscode. did a Ctrl+space after raw. and waited about 90 seconds, no new log entries.

hoechenberger · 2023-12-05T21:33:55Z

Thanks!

Can you check that you have the following settings in VS Code:

"python.analysis.indexing": true,  // this is the default, but check if it's really on
"python.analysis.userFileIndexingLimit": -1,

Lastly, you can set

"python.analysis.logLevel": "Trace"

to get debugging info – maybe this can give us some clues!

drammock · 2023-12-05T21:42:02Z

indexing was True
indexing limit was at 2000 (MNE has ~1800 files, so shouldn't have been a bottleneck?). I've changed it to -1
setting loglevel to Trace made the output pane explode with messages; I don't know where to start (dozens of lines per second, nearly continuously, too fast to read them)

I've also just enabled the "persist indices to disk for all 3rd party libraries" so maybe once it's gone through everything once it will actually become performant? 🤞🏻

drammock · 2023-12-05T21:47:55Z

it finally slowed down. every single message is info level. I searched for io/base and found these lines:

2023-12-05 15:39:08.324 [info] (2603285) [BG(1)] indexing: /opt/mne/python/mne/io/base.py [found 21] (7ms)

...

2023-12-05 15:39:12.858 [info] (2603285) [FG] parsing: /opt/mne/python/mne/io/base.py [fs read 1ms] (60ms)
2023-12-05 15:39:12.872 [info] (2603285) [FG] binding: /opt/mne/python/mne/io/base.py (14ms)

ctrl+space still shows "loading..." both before and after closing/reopening the IDE

hoechenberger · 2023-12-05T23:07:41Z

How did you install MNE?

drammock · 2023-12-06T00:30:29Z

editable pip install. -------- Original Message --------

…

On Dec 5, 2023, 17:07, Richard Höchenberger wrote: How did you install MNE? — Reply to this email directly, [view it on GitHub](#12250 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAN2AU543HCW5M2DJ2SNFBDYH6SMPAVCNFSM6AAAAABABSTQQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBRG43TENBUGU). You are receiving this because you were mentioned.Message ID: ***@***.***>

hoechenberger · 2023-12-06T07:32:18Z

Can you do a non-editable install?

After that one, I'm out of ideas…

cbrnr · 2023-12-06T08:57:20Z

FWIW, everything works fine for me, even with an editable install.

hoechenberger · 2023-12-06T09:33:07Z

FWIW, everything works fine for me, even with an editable install.

If the editable install is working for you out of the box, this indicates that your Pylance is actually not picking up the "installed" MNE, but rather the one in your current working directory / workspace. So this is not a valid test for either, editable and non-editable install. You need to use a workspace outside of the mne-python directory for testing.

cbrnr · 2023-12-06T09:37:42Z

Can we please switch to Hatchling ASAP? I keep forgetting that editable installs do not work with Setuptools...

And yes, installing it as non-editable in a new venv outside of the MNE source works for me.

hoechenberger · 2023-12-06T09:39:05Z

Can we please switch to Hatchling ASAP? I keep forgetting that editable installs do not work with Setuptools...

Sure but this still wouldn't resolve this issue here.
Pylance will always prioritize your workspace / CWD over the "global" installation.

As will Python…

cbrnr · 2023-12-06T09:41:00Z

OK, so as a dev I will basically never be able to check if these sorts of things are working? I am not going to roll a second venv just to test this.

hoechenberger · 2023-12-06T10:08:42Z

OK, so as a dev I will basically never be able to check if these sorts of things are working?

Errm … If you're running all your tests from within the mne-python directory, you cannot rely on anything if your actual goal is to test behavior of an actual installation. If you run python -c "import mne", Python will pick mne from your CWD in that case, not the one from site-packages.

This just to say that this problem affects all parts of Python package development. It's not isolated to type hints. It has nothing to do with using a venv or not.

The Python import system is just awful in this regard.

cbrnr · 2023-12-06T10:35:37Z

Yes, that's what CI should test, not any dev.

hoechenberger · 2023-12-06T10:40:18Z

So what was your point again? :D

drammock · 2023-12-06T15:35:54Z

FWIW, everything works fine for me, even with an editable install.

If the editable install is working for you out of the box, this indicates that your Pylance is actually not picking up the "installed" MNE, but rather the one in your current working directory / workspace. So this is not a valid test for either, editable and non-editable install. You need to use a workspace outside of the mne-python directory for testing.

update:

if I launch vscode from /opt/mne/python (where my editable install lives) I get "Loading..." for a very long time (this was already reported above)
if I launch vscode from some other directory, then I get "No suggestions" when I ctrl+space after raw.
if I do this:

$ mamba create -n typehints pip
$ mamba activate typehints
$ pip install git+https://github.com/mne-tools/mne-python.git@main

then launch vscode from anywhere (both ~/Desktop and /opt/mne/python work) then I get this:

I can even get the suggestions to work if I launch vscode from a terminal with typehints env active, and then switch vscode to mnedev after launching the IDE (which is weird / I don't understand why that works). The only thing that seems to not work is exactly the situation that I always use: launch vscode from a terminal where mnedev is activated (i.e. where mne is installed with an editable install).

hoechenberger · 2023-12-06T15:57:04Z

Thanks for testing, @drammock

I believe this will be resolved via #12269

Can you try with that branch?

drammock · 2023-12-06T17:46:45Z

I believe this will be resolved via #12269
Can you try with that branch?

Here's what I did:

$ cd /opt/mne/python
$ git checkout -b hatchling hoechenberger/hatchling
$ pip install -e .
$ code .

No completion, still "Loading..." forever. vscode's bottom bar shows mnedev as the active env

If I instead do code ~/Desktop then I do get completion, even if vscode's bottom bar shows mnedev as the active env.

Is that the expected pattern? Is there a chance that some cruft is causing the failure? in the root of my MNE clone I have (among other things) the folders .eggs, mne.egg-info, and .mypy-cache of which IDK if they might be stale/unnecessary.

hoechenberger · 2023-12-06T17:52:30Z

It's definitely not the expected behavior

Can you try to create a fresh clone of my branch and use that?

hoechenberger · 2023-12-06T17:53:30Z

Do you get completions with other Python packages?

Did the problem not occur before we merged this PR?

larsoner · 2023-12-06T18:17:33Z

Can you try to create a fresh clone of my branch and use that?

Or use git clean -xdf which will remove all files not under version control. I wonder if it's the mne.egg-info specifically, those can be problematic

cbrnr · 2023-12-06T18:18:54Z

This sounds like your VS Code installation is broken. Can you try to reinstall and maybe clean out your user and workspace (.vscode) settings?

drammock · 2023-12-06T20:38:13Z

Can you try to create a fresh clone of my branch and use that?

Do you get completions with other Python packages?

yes, e.g., numpy

Did the problem not occur before we merged this PR?

not sure, I didn't typically use this feature before

Or use git clean -xdf

Nothing changed after doing this.

This sounds like your VS Code installation is broken. Can you try to reinstall and maybe clean out your user and workspace (.vscode) settings?

git clean -xdf nuked the workspace settings. I looked at my global settings, nothing likely to affect this was found

Ultimately I did just fix it, by disabling the reStructuredText extension (which runs esbonio language server in the background). Not a big surprise, I've hit trouble with that extension before.

hoechenberger · 2023-12-06T21:10:51Z

reST is poision, I'm tellin' ya! 😅🥹

Great to hear things are working for you now!

drammock · 2023-12-06T21:15:33Z

rST is not the problem here. it's the vscode extension that relies on silently running sphinx builds in the background in order to offer rST completion hints for crossreferences. A small benefit that is not worth the complexity and cost.

hoechenberger · 2023-12-06T21:50:53Z

I was really just trying to make a joke :) But I do dislike reST, that's for sure :D
Good to know this extension causes problems, I had just recently considered starting to use it; now I probably won't

…ls#12250) Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Add type hints to read_evokeds(), read_raw()

9946e5a

hoechenberger marked this pull request as ready for review November 30, 2023 17:47

hoechenberger requested review from drammock, cbrnr, agramfort, mscheltienne and dengemann as code owners November 30, 2023 17:47

hoechenberger changed the title ~~Add type hints to read_evokeds(), read_raw()~~ Add return type hints to read_evokeds(), read_raw() Nov 30, 2023

This comment has been minimized.

Sign in to view

Merge branch 'main' into type-hints

26d0c9a

Add foundation for type hint testing through MyPy

699a456

hoechenberger requested a review from larsoner as a code owner December 1, 2023 12:58

MAINT: Add to CIs

62cd6d2

larsoner added 3 commits December 4, 2023 15:03

FIX: Simplify

fc668f7

FIX: Which

2496311

FIX: Comment

54ccf21

larsoner reviewed Dec 4, 2023

View reviewed changes

pyproject.toml Show resolved Hide resolved

larsoner and others added 3 commits December 4, 2023 18:48

Merge branch 'main' into type-hints

116c80c

Add changelog entry

9ebdc65

Fix typo

c1709e1

hoechenberger commented Dec 5, 2023

View reviewed changes

larsoner added 2 commits December 5, 2023 11:49

FIX: Ignore

aed55f5

larsoner approved these changes Dec 5, 2023

View reviewed changes

hoechenberger mentioned this pull request Dec 14, 2023

Add return type hints to read_raw_*(), read_epochs(), read_annotations() #12296

Merged

snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024

MRG: Add return type hints to read_evokeds(), read_raw() (mne-too…

390912b

…ls#12250) Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

MRG: Add return type hints to read_evokeds(), read_raw() #12250

MRG: Add return type hints to read_evokeds(), read_raw() #12250

Conversation

hoechenberger commented Nov 30, 2023 • edited Loading

This comment has been minimized.

larsoner commented Dec 1, 2023

hoechenberger commented Dec 1, 2023

hoechenberger commented Dec 1, 2023 • edited Loading

hoechenberger commented Dec 1, 2023

larsoner commented Dec 4, 2023

hoechenberger Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

hoechenberger Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

larsoner Dec 5, 2023

Choose a reason for hiding this comment

larsoner Dec 5, 2023

Choose a reason for hiding this comment

larsoner commented Dec 5, 2023

larsoner left a comment

Choose a reason for hiding this comment

drammock commented Dec 5, 2023

hoechenberger commented Dec 5, 2023

drammock commented Dec 5, 2023

hoechenberger commented Dec 5, 2023 • edited Loading

drammock commented Dec 5, 2023

drammock commented Dec 5, 2023 • edited Loading

hoechenberger commented Dec 5, 2023

drammock commented Dec 6, 2023 via email

hoechenberger commented Dec 6, 2023 • edited Loading

cbrnr commented Dec 6, 2023

hoechenberger commented Dec 6, 2023 • edited Loading

cbrnr commented Dec 6, 2023

hoechenberger commented Dec 6, 2023 • edited Loading

cbrnr commented Dec 6, 2023

hoechenberger commented Dec 6, 2023 • edited Loading

cbrnr commented Dec 6, 2023

hoechenberger commented Dec 6, 2023

drammock commented Dec 6, 2023 • edited Loading

hoechenberger commented Dec 6, 2023

drammock commented Dec 6, 2023

hoechenberger commented Dec 6, 2023

hoechenberger commented Dec 6, 2023 • edited Loading

larsoner commented Dec 6, 2023

cbrnr commented Dec 6, 2023

drammock commented Dec 6, 2023 • edited Loading

hoechenberger commented Dec 6, 2023 • edited Loading

drammock commented Dec 6, 2023

hoechenberger commented Dec 6, 2023

MRG: Add return type hints to `read_evokeds()`, `read_raw()` #12250

MRG: Add return type hints to `read_evokeds()`, `read_raw()` #12250

hoechenberger commented Nov 30, 2023 •

edited

Loading

hoechenberger commented Dec 1, 2023 •

edited

Loading

hoechenberger Dec 5, 2023 •

edited

Loading

hoechenberger Dec 5, 2023 •

edited

Loading

hoechenberger commented Dec 5, 2023 •

edited

Loading

drammock commented Dec 5, 2023 •

edited

Loading

hoechenberger commented Dec 6, 2023 •

edited

Loading

hoechenberger commented Dec 6, 2023 •

edited

Loading

hoechenberger commented Dec 6, 2023 •

edited

Loading

hoechenberger commented Dec 6, 2023 •

edited

Loading

drammock commented Dec 6, 2023 •

edited

Loading

hoechenberger commented Dec 6, 2023 •

edited

Loading

drammock commented Dec 6, 2023 •

edited

Loading

hoechenberger commented Dec 6, 2023 •

edited

Loading