-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
docdict and IDEs #8218
Comments
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.
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. |
ipython does not have this pb. I think it's an IDE fix that is needed here
… |
Wow, you're right! Great, this gives me something to work with. |
I've opened a feature request upstream at Pylance: |
Closing because there's nothing to be done on our end, it seems. @hoechenberger feel free to reopen if you disagree. |
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 microsoft/pylance-release#346 and leave a comment there, to show this issue is of relevance. |
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 The stubs are created fully automatically. I packaged them such that they can be installed via 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 |
Rather than a separate project if it's fully automated we could add a script to We could add a little |
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). |
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. |
Hello all, thanks for the positive feedback on the type stubs! For those who didn't follow the discussion: 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 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: to this (markup still subject to change): 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
Issues we're trying to address
Proposal
Additional tasks
Please share your thoughts! 👍 |
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? Besides that, amazing! (and apologies for the formatting, github on phone is not my preferred interface) |
Thanks for tackling this! Summarizing the discussion at today's dev meeting:
Personal notes from me (may not reflect opinions of others at the dev mtg):
|
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 🦠 |
Approx March 1 |
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? |
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 |
-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.. |
@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). |
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 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:
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. |
👍🏻 💯 |
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 |
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:
|
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? |
Running ipython in the IDE's terminal and typing |
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. |
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. microsoft/vscode-python#17172 |
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. Also this might improves it further: microsoft/vscode-python#17172 |
@mscheltienne that is also my experience, I can set it up that it works more or less, but there are so many annoying things that just don't work that it's currently not feasible to use IPython in VS Code. I don't want to use notebooks, so I'm stuck with the default Python interpreter, which does not have the Also, even if this worked, I'd still prefer to see the docstrings directly in the source code. For me, that's one of the points of working with an IDE. Or at the very least, have the docstring appear on hover, which is better than nothing. I guess we should put in the effort to provide the stubs, but AFAIR we hit some roadblocks and I don't know how we want to proceed. |
I don't want to re-iterate all of the points we've already addressed in this discussion, so it would be good to decide how to proceed. It seems like everyone was OK with integrating the stubs as a solution for displaying rendered docstrings in IDEs. Open questions:
|
how recently have you tried? The issue you linked to seems to be about windows specifically; for me on linux,
crossref to scientific-python/summit-2024#21. I'll be seeing Lars in a couple days and will hopefully learn from their approach and report back. |
@drammock here's the current experience I have with VS Code and IPython:
I did not encounter the double confirmation issue on my Mac, I think this might be specific to Windows (can't try that RN). If I'm doing something wrong, I'm happy to learn how this can be made to work. The ideal experience should be somthing like this (and that's how it works e.g. in PyCharm):
|
@cbrnr Why is the Python Interactive console not an alternative for you? Should that support everything you need? I think there might be a way to always show text-only reprs (instead of HTML reprs) if that's an issue |
I just don't like working with a Jupyter kernel, it behaves differently enough that I don't want to use it. |
Ah, interesting, I thought it's basically the same as IPython internally, just with a different frontend |
thanks for that. Here's what works for me:
|
At any rate ... 😅😅😅😅😅 I believe I can work on and finalize those type stubs next week or so. Please do ping me if you don't hear back from me until mid-June. 😃 |
notes from a conversation with @lagru about this yesterday: our situations / motivation is very different from the motivation in skimage. One idea Lars had was to have our docstrings fully populated within the source files, maintain a registry of which parameters in which functions we expect to remain in sync, and have a precommit / CI script that checks whether things that we think should remain in sync were have gone out of sync. It sounds like a lot to maintain (though if there's no docdict anymore maybe it's a wash?) but even so I don't think it will work for us because we have so many cases where we want param descriptions to be similar or almost identical but not exactly identical. TL;DR: @hoechenberger feel free to take inspiration from scientific-python/summit-2024#21 if it helps |
I don't really see how Lars's efforts are related to what I was trying to do here, which was merely expanding the docdict... but i might be missing something. I'm currently traveling and will look into this in more depth in a couple of days! thanks for pinging me anyway! |
I definitely like the idea of having our docstrings fully populated within the source files. This would solve the problem once and for all, since providing docstring expansions in type stubs is merely a workaround. Of course we'd need to decide whether the necessary tooling is feasible, but if so I'm all for it! |
I was assuming that although the end goal is different, some of the machinery could probably be repurposed fairly easily. But maybe not... just wanted to make sure you knew about it in case it saved some effort. |
Yep, absolutely, and I intend to take a look! Thanks for letting me know! |
@cbrnr et al, FYI here's a crossref to the wider community discussion scientific-python/summit-2024#27 |
FYI, this is now fixed in VSCode: https://code.visualstudio.com/updates/v1_91#_support-for-restructuredtext-docstrings |
This is still an experimental feature and I believe @hoechenberger has already played around with it, but without much success (right?). Also, I think this only covers RST and not Numpydoc style (which is more than just RST formatting, e.g. parameter types etc.). But yes, I really hope that one day our docstrings will be properly rendered (we only need to replace the docdict, which is a separate but related issue). |
yep. but good that they're working on it
I think that's right. But again, progress is being made.
yep, this means that once our docdict is gone, things will render nicely on hover. |
FYI Pylance produces lots of error messages like "ERROR: Pylance failed to parse docstring at index 86" when it encounters "docdict'ed" parameters. |
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:
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?
The text was updated successfully, but these errors were encountered: