mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

docdict and IDEs

Open hoechenberger opened this issue 4 years ago • 50 comments

While the docdict simplifies our lives and makes documentation more consistent by reducing redundancy, there's also an unfortunate side effect; namely, that IDEs may be unaware of our custom approach to docstrings. I'm often confronted with information like this:

Screenshot 2020-09-07 at 16 46 49

which, really, is just as unhelpful as it can be if I'm not sure about the parameters the function expects.

I thought maybe we could provide some stubs of the documentation with those placeholders expanded; but apparently stubs are merely intended for type information.

Now I was thinking that maybe when preparing a release, we could expand those strings? So that at least the MNE version that most users will be using – a released "stable" version – would have "proper" docstrings?

Or… are there any ways to kind of inject the rendered HTML docs from our website or something? Anyone know about magic of that kind? @drammock maybe?

hoechenberger avatar Sep 07 '20 15:09 hoechenberger

maybe when preparing a release, we could expand those strings?

That seems like it would fix the issue you're having. I think it would have to be an automated thing that expanded all the docdicts and made a commit, made a release, and then did a revert commit right after the tagged release to put the docdict back in place. Note that this would not fix your issue if you're using latest master in your IDE.

are there any ways to kind of inject the rendered HTML docs from our website or something?

Seems like that's really an issue for the IDE devs, not for MNE-Python devs. We can't control where they're looking for docs, and it appears they're pulling straight from the source code files.

I suppose if there were a more widespread standard for dealing with docstring redundancy, and that standard were already supported by various IDEs, then I'd consider switching from docdict to whatever that mystery other standard is. But off the top of my head I don't know of one.

drammock avatar Sep 07 '20 21:09 drammock

ipython does not have this pb. I think it's an IDE fix that is needed here

agramfort avatar Sep 08 '20 07:09 agramfort

ipython does not have this pb.

Wow, you're right! Great, this gives me something to work with.

hoechenberger avatar Sep 08 '20 07:09 hoechenberger

I've opened a feature request upstream at Pylance: https://github.com/microsoft/pylance-release/issues/346

hoechenberger avatar Sep 09 '20 08:09 hoechenberger

Closing because there's nothing to be done on our end, it seems. @hoechenberger feel free to reopen if you disagree.

drammock avatar Sep 09 '20 12:09 drammock

Just wanted to let you know that this issue still has not been addressed upstream. If you have a minute, it would be great if you could add a 👍 to https://github.com/microsoft/pylance-release/issues/346 and leave a comment there, to show this issue is of relevance.

hoechenberger avatar Jan 30 '21 09:01 hoechenberger

I took a step forward here since I don't think we'll ever see a fix in the VS Code Python extension / Pylance. The problem is that Pylance and other language servers only perform static analysis without actually running or importing the code. This makes it impossible for these tools to get access to "expanded" (dynamically generated) docstrings like the ones we're using.

It's gotten so bad that for most MNE functionality, the docstrings are now completely useless to me within the IDE. I have to keep a browser window or an IPython window open to view the docs.

I therefore took the initiative to create a script that generates type stubs for all public functions and classes in MNE and injects (expanded) docstrings into the stubs. These docstrings will then be picked up and displayed correctly by Pylance in VS Code (I didn't test any other tools so far, but @cbrnr told me that it seems to be working nicely in PyCharm as well).

This is what MNE-Python main looks like for me out of the box: Screenshot 2023-11-25 at 00 26 22

And here with the stubs: Screenshot 2023-11-25 at 00 25 41

The stubs are created fully automatically. I packaged them such that they can be installed via pip. Hence, no modifications to MNE are required. There is no interference with any MNE-Python code at runtime.

Currently, default parameter values are still missing. That's next on my to-do list.

You can find the stubs and installation instructions at https://github.com/hoechenberger/mne-python-stubs

cc @larsoner @mscheltienne @britta-wstnr @wmvanvliet

hoechenberger avatar Nov 24 '23 23:11 hoechenberger

Rather than a separate project if it's fully automated we could add a script to tools/dev/* that we run manually before each release to generate the stubs. (We could do things in a more automated way, too, with a bit more work.) Then there's no need for a separate package. We already have a step like this with generate_codemeta.py.

We could add a little .yml to at least ensure the script runs on every PR (or run it on a schedule). It would be really neat to create an action that runs to recreate the stubs and pushes any changes to an open PR. That would be more work but could be a neat Sunday project for someone who is motivated.

larsoner avatar Nov 24 '23 23:11 larsoner

This is great! I agree that it should probably be integrated into MNE, since it runs automatically anyway.

I would not run it on every PR though, because it takes a non-trivial amount of time (and resources).

cbrnr avatar Nov 25 '23 06:11 cbrnr

This is amazing @hoechenberger - and I agree with the other posters that it should be integrated into MNE for (at least) releases. I assume many users use VSCode or PyCharm so this is an important enhancement.

britta-wstnr avatar Nov 27 '23 19:11 britta-wstnr

Hello all,

thanks for the positive feedback on the type stubs!

For those who didn't follow the discussion: I've created "stub files" (.pyi) that contain the "expanded" docstrings for all MNE-Python functions, classes, and methods.

The stubs do not contain any type hints that are not already present in MNE-Python, except for a few that are plainly obvious to stubgen (the tool I used for creating the stubs) and were added automatically.

The goal was to improve the user experience when working in an IDE.

With these type stubs, the VS Code experience changes from this unusable mess: Screenshot 2023-11-28 at 21 08 07

to this (markup still subject to change): Screenshot 2023-11-28 at 21 09 20

I am happy that you like it!

I have worked and talked with @cbrnr for many hours in the past couple of days to evaluate what could be a viable move forward from here. I will try to summarize our thoughts below and make a concrete proposal on how to proceed.

Background information

  • The stubs are currently optimized for the best experience in VS Code.
  • VS Code (or, rather, Pylance / Pyright, which are used by VS Code) cannot handle Numpydoc-style docstrings and basically has no support for reST-based markup.
  • Hence, I inject Markdown into the stubs in many places, and remove reST markup in others, in order to make things look better.
  • However, PyCharm doesn't support Markdown.
  • These stubs would therefore actually degrade the UX for PyCharm users!
  • At the same time, PyCharm does support Numpydoc-style docstrings pretty nicely out of the box.
  • So there's a conflict: what works well in VS Code doesn't work well in PyCharm, and vice versa.

Issues we're trying to address

  • But the biggest issue we set out to solve is, of course, that of the "un-expanded" doccer-based docstrings (e.g., %(verbose) etc.)
  • Second-biggest issue: Sphinx tags / blocks often simply disappear in both VS Code and PyCharm. For example, blocks like .. warning::, .. versionchanged:: etc. just don't show up at all!
  • The third issue is aesthetics when it comes to Sphinx-based references: reading ~mne.io.Raw is irritating with that leading tilde.

Proposal

  • We create type stubs with expanded docstrings from which we remove Sphinx tags / directives that yield hidden blocks and ugly markup (e.g., :class:, ~, .. warning:: etc). These stubs will automatically look good in PyCharm, and provide more useful documentation in VS Code (albeit it won't be rendered nicely in the latter IDE).
  • These type stubs live in a package types-mne that is separate from MNE-Python. This allows us to release this package independently of MNE-Python releases. This package should live under the mne-tools org. Having a package separate from mne also avoids unintended interference with the existing .pyi files when automatically creating the stubs.
  • While normally such stub packages depend on the package they're providing the stubs for, we could instead make mne depend on this package, so it gets installed automatically when you install MNE-Python.
  • VS Code-specific alterations to the docstrings should live in a separate package types-mne-vscode. This is where all the Markdown and emoji fun can happen.

Additional tasks

  • We'd also need to evaluate behavior of Spyder and Jupyter Notebook.
  • @cbrnr and I will reach out to pyright and inquire about the possibility of supporting Numpydoc (or if a contribution to add support would be appreciated).

Please share your thoughts! 👍

hoechenberger avatar Nov 28 '23 20:11 hoechenberger

These type stubs live in a package types-mne that is separate from MNE-Python. This allows us to release this package independently of MNE-Python releases. This package should live under the mne-tools org. Having a package separate from mne also avoids unintended interference with the existing .pyi files when automatically creating the stubs.

I'm not entirely sold on the separate package, the only.pyi files are the init for lazy loading right? And I don't see the need to release separately from MNE, on the contrary it might create some conflicts if you include dev changes?

Besides that, amazing! (and apologies for the formatting, github on phone is not my preferred interface)

mscheltienne avatar Nov 29 '23 06:11 mscheltienne

Thanks for tackling this! Summarizing the discussion at today's dev meeting:

  • getting this to "just work" for users should be a high priority for 1.7 release. @hoechenberger and @cbrnr please feel free to proceed and pester us for help (design decisions, implementation, testing, whatever) at your discretion.
  • There was a general sense that it might be better if the solution were not a separate package but instead lived inside MNE-Python, but for now it seems OK to develop as a separate package(s) if that's easier; once things are progressed we can see how hard it would be to migrate it into MNE-proper.
  • there's already an issue about pylance's failure to support rST here: https://github.com/microsoft/pylance-release/discussions/2677; everyone please upvote it.

Personal notes from me (may not reflect opinions of others at the dev mtg):

  • please don't inject extra emoji into the docstrings. Other major scientific packages have headings that say e.g. "Parameters" not "🛠️ Parameters", and doing our own special thing here is not an improvement IMO
  • I'll mention rst2html5 as something that might be useful to look at, as well as MyST markdown (which aims to be as expressive as rST)... both may be irrelevant if vscode doesn't support MyST or if rst2html5 doesn't expose any low-level parsing functionality (I've only ever used it for high-level stuff)

drammock avatar Dec 01 '23 23:12 drammock

Thanks for the feedback and summary, @drammock!

I discussed with @cbrnr and I believe we have a solution that we'll be able to integrate into MNE.

I'm currently bedridden though thanks to 🦠 So I cannot tell when exactly I'll be able to pick up work on this again. Could you kindly remind me when we anticipate to release MNE 1.7? Thanks 🙏

hoechenberger avatar Dec 02 '23 09:12 hoechenberger

Could you kindly remind me when we anticipate to release MNE 1.7?

Approx March 1

drammock avatar Dec 02 '23 13:12 drammock

While we're at it, could we maybe also try to minimize docstring expansions? I've seen a couple of docdict entries that occur only twice in the entire codebase, and IMO this is not enough to warrant making the entire docstring unreadable when looking at the source. I'm sure I am not the only one consulting the source directly, so even without the stubs I think we can improve the situation by including only docstrings that occur at least e.g. three times (preferably four times) in the source. WDYT?

cbrnr avatar Mar 18 '24 06:03 cbrnr

Also, I'd argue that we need the stubs package with expanded docstrings not only for releases. After all, developers are going to benefit, and they are working with the current main branch. But I guess the stubs package created for a particular release can also be used with the main branch (which is based on that release), right?

cbrnr avatar Mar 18 '24 06:03 cbrnr

Also, I'd argue that we need the stubs package with expanded docstrings not only for releases. After all, developers are going to benefit, and they are working with the current main branch. But I guess the stubs package created for a particular release can also be used with the main branch (which is based on that release), right?

We can bound the dependencies accordingly such that you'll be able to use the stubs with latest stable + current dev

I wouldn't want to do it without any kind of upper bound on the MNE version though, as the more time passes, the more out of date the stubs will be

hoechenberger avatar Mar 18 '24 08:03 hoechenberger

-1 on reducing duplication by minimizing docstring expansions for entires that occur only 2 or 3 times, with time those entries will fall out of sync.. +1 on having the stubs available for main!

mscheltienne avatar Mar 18 '24 08:03 mscheltienne

@mscheltienne honest question, how do you read docstrings? Do you consult the API docs at mne.tools? Or do you use IPython? Do you just follow the docstring expansions to their source? I'm genuinely wondering, because the docstring UX with MNE dev in VS Code is really bad (and I'm not blaming VS Code).

cbrnr avatar Mar 18 '24 15:03 cbrnr

I agree that the situation with main is not satisfactory. In my case, I think I end up looking on the online API documentation often, Sometimes I also query a docstring through IPython or through a search in the codebase for ["xyz"] yielding the docdict entry for this element.

I'm still not fluent in VS Code, having switched only a couple of months ago, but IMO, the options you have to navigate the codebase/look the docstrings are:

  • Ctrl + click to go to the declaration -> pointless with MNE-main as the docstrings are un-expanded
  • Through Pylance integration, e.g. hovering the mouse over a definition -> works great if you have the stubs
  • IPython
  • Follow the docstring expansions to their source, i.e. find the docdict entry -> not great for certain entries which are 'constructed' from other entries, e.g. picks
  • Online API documentation (outside of VSCode)

Any other ways to read/find a docstring?


Anyway, if we limit the docstring expansions to e.g. 4+ occurrences, then the go to definition approach becomes more viable, but on the other hand, duplicated docstrings will fall out of sync with time which will decrease the overall documentation quality.

My opinion is that our users are not developers and rely heavily on the online API documentation, on the online tutorials, on IPython and almost never go to a function/object definition. IMO, the developer benefit of having less expanded docstring is outweighted by the documentation quality which benefits most of the users.

mscheltienne avatar Mar 18 '24 16:03 mscheltienne

My opinion is that our users are not developers and rely heavily on the online API documentation, on the online tutorials, on IPython and almost never go to a function/object definition. IMO, the developer benefit of having less expanded docstring is outweighted by the documentation quality which benefits most of the users.

👍🏻 💯

drammock avatar Mar 18 '24 18:03 drammock

I think we should also try to make life easier for developers. After all, the entire package depends on people contributing mostly in their free time, so this take is a bit short-sighted IMO.

The argument regarding consistency is also flawed, because there are instances where functions simply contain incorrect docdict entries, e.g. https://github.com/mne-tools/mne-python/blob/maint/1.7/mne/time_frequency/tfr.py#L3719-L3756, where the shape of the data array is incorrect (it's taken from AverageTFRArray). So clearly, the trade-off between consistency and readability is not that clear-cut, and I'd vote for getting rid of docstring expansions every time.

signal-2024-05-16-162146_002

cbrnr avatar May 16 '24 15:05 cbrnr

I think we should also try to make life easier for developers. After all, the entire package depends on people contributing mostly in their free time, so this take is a bit short-sighted IMO. The argument regarding consistency is also flawed, because there are instances where functions simply contain incorrect docdict entries

You're not wrong. The docdict does cause developer inconvenience, and does not guarantee correctness or even consistency. But consistency is better now with docdict than it was before it was added, and probably could be made better still by some thoughtful improvements to how we implement/use it.

Offline @larsoner mentioned to me that the initial adoption of the docdict led to a lot of incorrect/outdated docstrings getting corrected. I am working under the assumption that if we nix the docdict, errors and inconsistencies will gradually creep back in, and that this should be avoided.

In other words, I still agree with @mscheltienne that the situation involves a trade-off, and that the net inconvenience to developers is outweighed by the net advantage to users. By saying this view is "short sighted" it sounds like you think the docdict inconvenience is bad enough to drive away or deter contributors/maintainers. Is that right?

Meanwhile, getting back to the original thrust of this issue:

  • support for rST / numpydoc style docstrings in VSCode is progressing: https://github.com/microsoft/pylance-release/issues/5363
  • in the meantime (or if that effort doesn't pan out): if the stub generation can be automated, would it work to have separate packages for vscode and pycharm, generated by different scripts?
    • If we go that route I'd say it's better to have those packages depend on MNE instead of the reverse (so that users can choose to install them or not depending on which IDE they use).

drammock avatar May 31 '24 20:05 drammock

I don't disagree that this is a trade-off (basically all choices we make are trade-offs), but I disagree that the current approach is a net benefit. I'm working with the source in an IDE, and I cannot read many docstrings. My IDE cannot resolve those template entries either, so I'm basically forced to always check the API docs on our website, which is really really annoying. I don't think this is such an exotic workflow?

cbrnr avatar May 31 '24 20:05 cbrnr

I'm basically forced to always check the API docs on our website, which is really really annoyin

Running ipython in the IDE's terminal and typing mne.time_frequency.EpochsTFR? also works and I find that easier than the website, FWIW.

drammock avatar May 31 '24 21:05 drammock

I don't use IPython because its integration in VS Code is unfortunately non-existent 🤷.

cbrnr avatar May 31 '24 21:05 cbrnr

I don't use IPython because its integration in VS Code is unfortunately non-existent 🤷.

there's an integrated terminal, in which you can run arbitrary commands, e.g. ipython.

drammock avatar May 31 '24 22:05 drammock

there's an integrated terminal, in which you can run arbitrary commands, e.g. ipython.

Of course, but I said "its integration [...] is non-existent", which means that things like executing (selected) line(s) of code (usually bound to Shift-Enter) doesn't work. This includes absolute basic things like indentation is not transferred correctly, and Shift-Enter does not actually execute the selection, because the pasted code contains an empty line at the end, which has to be confirmed with another Enter. https://github.com/microsoft/vscode-python/issues/17172

cbrnr avatar Jun 01 '24 09:06 cbrnr

I don't use IPython because its integration in VS Code is unfortunately non-existent 🤷.

At some point I had my interactive window set as IPython in VSCode. When I would hit Shift+Enter on a code snippet in the editor, it would run in an IPython console in the terminal, and if it was not running it would open one. It would handle indentation/line endings. It wasn't perfect, especially I would have to start using it either (1) with no other IPython console open with Shift+Enter, or with the IPython console opened with Shift+Enter in focus; and in the end I switched back to VSCode default ipykernel based interactive windows. I'm not finding anymore the post that explained how to set it up, IIRC, it was on SO.

Also this might improves it further: https://github.com/microsoft/vscode-python/issues/17172

mscheltienne avatar Jun 01 '24 09:06 mscheltienne