Refactor mne.sys_info()
#11544
Refactor mne.sys_info()
#11544
Conversation
|
The reason I removed the sections (I know it was intentional) is that it looks messy when packages are not installed. It also depends on which packages are installed/not installed whether or not there is a newline at the end of the output. |
Sure but on the flipside now it looks messier for our most common / recommended installs (i.e., graphical installers or even So still -1 on me for removing the newlines
This is a behavior that could be changed by refactoring how/when the newlines are printed (i.e., only print one if you are about to print a new output "section") |
|
WDYT about this: instead of giving one line to each "not found" package, there is a new section at the end that says The reason I suggest it is that, IMO, it is useful information: they can see that and think "oh, there are many optional packages, maybe the thing I'm trying to do needs one of them?" Put another way: if we simply omit the not-found packages, then the sys-info output usually (always?) looks "complete" as if there were nothing wrong. |
|
To combine both of your suggestions, could we create section headings (e.g. "Required packages", "Developer packages (optional)", etc.) to make it clearer? If a section is completely empty, I would not show the heading either. Instead, we could have a final section ("Not installed packages (optional)" or similar) and list all of those packages in one line. I would try to avoid making it look as if missing optional packages indicates that something is wrong – MNE should work without any optional package. |
yes, but define "MNE should work". Not everything will work after Personally what makes the most sense to me is 5 sections:
But adding the dev dependencies will make this list longer not shorter. Maybe a compromise would be
this proposal does not accept your idea @cbrnr of omitting entire sections if all packages in it are not installed. Again: IMO it's a useful discovery mechanism to see what could be there and utilized as well as what is already there and being utilized. |
|
We already have a dev dependencies section enabled with -d from the command line (and something similar in the python interface). I think we already have sections close to the proposed organization, +1 for improving it and/or adding headers to make it clearer |
|
@hoechenberger I just saw that you were also working on improving the output of |
I don't think so, the use was just a bit excessive and I've toned it down since. Need to post a new screenshot and an explanation / justification. Maybe later today or sometime tomorrow |
|
@drammock I agree with you that additional dependencies are necessary to make certain features of MNE work. I like the idea of having sections with titles, and as @larsoner has mentioned we have a switch to toggle dev dependencies (off by default). So the list shouldn't get longer in my case, and only a bit longer in your case (but it's more clearly structured). I was wondering if there was a way to parse the dependencies from our |
|
@hoechenberger and what is your opinion on the other points discussed here (i.e. shortening the list of not installed packages by putting them all on one line, adding section headings, single-sourcing dependencies)? |
+1 on this |
FWIW here's what I do when generating package metadata: mne-python/tools/generate_codemeta.py Lines 101 to 113 in bf25021 I think this would change (I think easier?) when we switch to |
|
It will probably simplify when using But it should be possible with |
|
This requires python 3.8, our current min version is 3.7 In [8]: importlib.metadata.requires("mne")
Out[8]:
['numpy>=1.15.4',
'scipy>=1.1.0',
'matplotlib',
'tqdm',
'pooch>=1.5',
'decorator',
'packaging',
'jinja2',
'h5io; extra == "hdf5"',
'pymatreader; extra == "hdf5"',
'pytest; extra == "test"',
'pytest-cov; extra == "test"',
'pytest-timeout; extra == "test"',
'pytest-harvest; extra == "test"',
'flake8; extra == "test"',
'flake8-array-spacing; extra == "test"',
'numpydoc; extra == "test"',
'codespell; extra == "test"',
'pydocstyle; extra == "test"',
'check-manifest; extra == "test"',
'twine; extra == "test"',
'wheel; extra == "test"',
'nitime; extra == "test"',
'nbclient; extra == "test"',
'sphinx-gallery; extra == "test"',
'eeglabio; extra == "test"',
'EDFlib-Python; extra == "test"',
'pybv; extra == "test"',
'imageio-ffmpeg; extra == "test"'] |
|
I was just about to post this 😄 – and I was going to say that it doesn't reflect all our requirements files (at least |
|
see also https://stackoverflow.com/a/70725798 about using |
|
And there's obviously a discrepancy between the packages output by |
|
Re Python 3.7, I have no problem bumping our minimum version to 3.8. SciPy is preparing for 3.9 on |
|
yeah, py3.7 EOL is 2023-06-27 so we should definitely bump |
|
Here's the new output (the ✔︎ and ✘ symbols are only included if the terminal supports Unicode): |
|
With |
|
BTW, aligning strings with the checkmark using I think having the checkmarks (when possible) is worthwile, because they make it clear that a listed package is installed. We could think about adding a new parameter if someone really wants to explicitly change that. |
|
OK, I think the new output looks quite nice already, but now it's question time. With the new way of determining dependencies with And a quick unrelated note: maybe we should enforce line wraps? WDYT? |
|
It seems we're hitting this issue, which has already been fixed (but there's no release yet). Not sure why |
|
Nope, that didn't fix it. |
|
Pushed a commit to ignore the dep warning coming from other package imports (we could also put it at the level of the On my system I get: The |
|
this aspect of @larsoner's output is weird to me: why does it get a checkmark if it's version is unknown/undetermined? (aside: I'm still -1 on using emoji at all, I just don't see what it adds here) |
|
Output looks reasonable for me: Output |
| @@ -198,23 +197,20 @@ stages: | |||
| - bash: | | |||
| set -e | |||
| mne sys_info -pd | |||
| mne sys_info -pd | grep "qtpy: .*{PySide6=.*}$" | |||
There was a problem hiding this comment.
It's really useful to have qtpy in there and the backend used when debugging people's info, so I'd like to see this come back in / be adjusted in some form rather than be removed
| @@ -7,4 +7,4 @@ pooch>=1.5 | |||
| decorator | |||
| packaging | |||
| jinja2 | |||
| importlib_resources>=5.10.2 | |||
| importlib_resources>=5.10.2; python_version < '3.9' | |||
There was a problem hiding this comment.
Might need to be 3.10, or matplotlib has a bug? Not sure
https://github.com/matplotlib/matplotlib/blob/5b1e02d734245a8f39c1c057f04539ab1c94738c/setup.py#L337
There was a problem hiding this comment.
It depends on which functionality you need. The version in the standard library lags behind the PyPI package: https://pypi.org/project/importlib-resources/
There was a problem hiding this comment.
We need importlib.resources.files, which was added in 3.9: https://docs.python.org/3/library/importlib.resources.html#importlib.resources.files
| @@ -0,0 +1,2 @@ | |||
| mne-qt-browser | |||
| PySide6 | |||
There was a problem hiding this comment.
I don't think this should be added here but rather qtpy.
If we had to pick one PyQt6 would be better anyway until PySide6 fixes the matplotlib-interactive-doesn't-work bug
I take that back actually. In addition to I don't see |
|
Re some packages missing (such as |
Taking a step back, I don't think this should be the inclusion criteria/guiding force here. You seem to be making multiple changes (?):
(1) and (2) are great but (3) I don't think is a good idea. Or at least should be done separately, since changing the install options should be out of scope for this PR. The point of The most important thing to me is not losing these behaviors / this info. I already consider it a loss that we will now need to do extra work to get all the useful information compared to before. Now At the end of the day we want info from a bunch of modules that are not dependencies, including stuff like I found the curated groups we had before easier to parse and think about for the purposes the function is meant to serve. It might be easier to go back to that method and just improve the printing / organization...
... the curated list we have on |
You really want to duplicate and/or manually curate yet another list of packages? I was assuming (maybe mistakenly) that parsing all relevant packages automatically would be preferred. And yes, this PR evolved a bit from mere cosmetics to trying to organize our dependencies. |
I honestly don't think it's worth the hassle unless it's trivial to do (which apparently it's not) |
Yes for now, at the very least for categorization. Maybe someday we want If you're worried about this and want to check consistency in the meantime, you could create a dict of heading->modules in |
|
Looking at |
Now that I put in a decent amount of time it is trivial to do, but not possible because we want to include packages in OK, I'll revert to the hand-picked list of packages then and remove all attempts to parse anything from our requirements. |
|
Superseded by #11568. |
This shortens the output of
mne.sys_info()such that modules that are not installed are not printed in the output. I've also fixed some minor things like importingmultiprocessingat the top (because it is available in the standard library) and removing the example (because this was outdated and it's probably not worth to change it everytime the function changes, plus I think it doesn't add any value).Fixes #11543.
To do
dependenciesparameter.Add newuse_unicode=Trueparameter?Add an option to wrap lines?