-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
DOC: Auto-populate related software using mne-installers yaml #12731
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -0.5 on this change. Seems like replacing ~20 lines of maintained text with ~200 lines of maintained code. Also I don't love the fact that folks will now need to install a bunch more packages to build the docs, just so that we can print the name, URL, and description of those packages on a webpage (i.e., not because they're needed for our tutorials).
Final point in opposition: previously to add something folks would just make a PR to MNE-Python. Now, they must package their software for PyPI and conda-forge, and make the PR to some other repo which they may not understand well (recipes? constructors? etc).
Is there some middle ground here? Like maintaining a JSON or YAML file that gets read in by the sphinx directive?
Thanks for the quick review @drammock ! I think there are a few misconceptions that might help to clear up:
Well a decent chunk of the code is doing such a parsing of a YAML file already from mne-installers, which was an idea +1'ed in #11831 (comment), and is also why this would close mne-tools/mne-installers#104. So in this sense yes it does already implement a YAML parsing. We could simplify it (e.g., get rid of the
this would be super easy to parse and would cut out probably 20-30 lines. Another 40ish lines is the Sphinx directive part, which you'd have in a simple solution. Then maybe 100 lines are the
They don't! Only CircleCI needs to do this. You can build docs right now on my branch and whatever packages you have will have the I think it's a big benefit actually that CircleCI must install these packages to get their info. Currently, we have no validation that the packages we suggest to people are even installable -- and some aren't at the moment, and this PR revealed it in local testing. So with this PR we have constant checks that whatever we're telling people to install can actually easily be installed.
This part was an opinionated choice I made. My thinking was that a reasonable bare minimum for inclusion in the list is that your package should be on PyPI, as it shows that you've at least met a minimal requirement for thinking about maintenance etc. If you buy this, I figured conda-forge wasn't far behind but I agree could be too much -- I can reword it to say just PyPI. (It does need to be on conda-forge to land in our installers, though, so it would be nice to encourage people to conda-forge-ify their packages in text somewhere at least!) But if you don't buy this argument about needing to be on PyPI, ~50 lines in this PR are a
All of the mechanics for resolving these are already in the current code, it's just a matter of us choosing what should be required. And while this does make it a little bit harder to add a package to the list, it's not a ton harder, and pushes people hopefully toward including their packages in our installers for example. |
@drammock I pushed a commit to simplify the parsing along the lines I suggested above, feel free to look and see if you're convinced it's less painful / more useful now |
TL;DR: right now I'm leaning toward decoupling this "related software" list from "extensions shipped with our installer", and using the code in this PR to address mne-tools/mne-installers#104 but putting the resulting list on a different page. LMKWYT
Indeed this was a misunderstanding
agreed, I see your point
IMO:
if we do end up parsing this list out of the installer YAML, then I like this idea. But see point 2 above.
I'm OK with saying "must be on PyPI to make this list" and I do think that requiring conda-forge also is too high a bar; suggesting conda forge (and linking to |
I think we need two lists, one for related neurosciencey software, and a different one for everything the installers install, as they answer different questions for people. The second list is trivial to make -- parsing the installer YAML is < 10 lines of code, so it's not difficult to pull things out. You get it almost for free if you make the first list partially based on the installer YAML. So I think we need to decide if parsing the installer YAML for the "related software" list makes sense first.
I don't think they need to be identical, but I think that the list in MNE-Python should be a superset of the list of neurosciencey stuff we put in the installers. After all, if a neurosciencey thing is in our installer, it should be related (somehow) to what MNE is used for. But going the other way, if a package is related to MNE, yes we should ideally include it our installer, but we can't always do it (not on conda-forge, artifact limitations, etc.)! And as far as "neurosciencey" or not it's easy enough to make this decision at the time of addition to MNE-Installers by which section we put it in if we merge mne-tools/mne-installers#285. But even if you don't agree with this part or find that line of thinking constructive, reading the installer YAML in this PR should result in less work/effort and a more complete list -- this PR lists 44 packages and And importantly, in the end I don't see it any tangible drawback to inspecting the installer YAML for package names -- it only uses a few lines (it's < 10 now!) and has some tangible benefits.
We haven't hit that in a while because we improved our artifact removal policy. But even if at some point we have to be more selective or have a higher bar in terms of what we include in the installers... they should still remain as consistent as possible. Neurosciencey things in the installer should be in our related software list here (the "superset" idea), which is what this PR helps ensure. |
Yep agree
This is the part I'm struggling to fully agree with. To me the bar is much lower to say "FYI here's a related package" than to say "we're installing this on your system". I'm other words I want to leave open the possibility of cases where we can but choose not to include a package in our installer even if we are ok putting it on this list. That said: I can see that your code supports that situation. So my main concern is how we describe this mechanism publicly to package authors. We shouldn't give the impression that all it takes is a PR and we'll blindly start shipping their code to our users. |
That's fine with me -- there are already some packages that we don't install because we can't. I think it's okay if we decide there are some that we shouldn't (for whatever reason). It's consistent with the superset idea in this PR anyway!
I think this can be mostly orthogonal to this PR. To separate it, rather than make any suggestion of "you should add your package to PyPI, conda-forge, then our installers!" here, we can just say for now "modify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I'm convinced / happy with where this has landed. See my inline comments for the last few nits.
Co-authored-by: Daniel McCloy <[email protected]>
Okay thanks for working through it with me @drammock, I think it's much cleaner now than what I originally had 👍 Commited fixes and moved to |
Related to mne-tools/mne-installers#104
Closes #11831
recipe.yml
importlib.metadata
to get URLs and descriptions (DRY; we don't need to maintain them, if they are bad, packages should fix their packaging!). I considered using conda-forge info or scraping PyPI, but it's easy to get this info if packages are installed -- so install them on CircleCI usingpip
(or maybe eventuallyuv
or whatever!) and use that infopip install mne[doc_related]
developer / CirlceCI option to facilitate CircleCI installing the necessary packages. This has the side benefit of making sure that all packages are installable. Can trivially be moved totools/circleci_dependencies.sh
if it's too visible inpyproject.toml
.CircleCI
go through this pain, but local devs building the docs won't hit errors (will just have incomplete info in the list, which they probably won't ever notice and won't affect their dev process)This way we make sure that our list stays up to date. Dependencies that can't be installed will be flagged implicitly by CIs. Updates to mne-installers will trigger errors, so ideally when we add stuff there we should add it here simultaneously. But if we forget, we'll get a red CI with a decent error message telling us to update stuff.
Not 100% sure I got the
doc_related
optional dep list correct since I derived it by running locally (where I might have already had stuff installed), but CircleCI should tell us quickly if it needs to be updated.