-
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
ENH: Warn about versions in sys_info #12146
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.
thanks for doing this! My (bikeshedding) nitpicks are:
- prefer to have the message on the same line as the reported version (similar to how we put BLAS info in parentheses after reporting numpy version)
- instead of an extra check/x-mark in the message, I would consider having the main left-margin checkbox be the indicator, like
- ✓ (installed stable or dev)
- x (installed old version)
from urllib.error import URLError | ||
from urllib.request import urlopen | ||
|
||
from packaging.version import parse |
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 surprised this passed style checks, shouldn't it fail on import sorting grounds?
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.
This makes it a different import block
Actually this split is intentional. It gives people two chances to see the version mismatch. If you really want them EDIT: in one place, I guess I'd prefer and can live with them both in a separate message at the bottom since it's more visible than when it's up by |
I wasn't being clear. I like that you add a warning-like sentence after the whole results table is done. What I meant was that I don't like the entry for MNE spanning two lines. |
So something like:
and for the other variants:
|
yes exactly! |
Okay pushed a commit for that. Just need to add a couple tests and changelog! |
Okay tests and changelog updated |
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.
LGTM except for one oversight I think, see below
Co-authored-by: Daniel McCloy <dan@mccloy.info>
We could also query PyPI to ensure the package is already pip-installable by the time we display the message: |
@larsoner I'm behind a corporate firewall that injects a self-signed MITM certificate so it can spy on TLS connections Which can cause troubles connecting to online services unless one explicitly specifies that one trusts this cert For example, for Python's I'd like to try out how this branch behaves in different settings (with and without the env var set; connection from the office and remotely through a VPN; on the host machine and inside a Docker Dev Container) before we merge, just to ensure sys_info doesn't suddenly explode or spit out many lines of error messages But I won't have time to test before next week Please hold off merging until then if possible And please do ping me if you don't hear back from me by Wednesday Thanks |
Good idea, IMO there is no rush on this one |
Seems to be working for me, however, I basically immediately see this line when running behind our corporate firewall:
Clearly, 2 sec have not passed, I get this output almost instantaneously So maybe we should just remove the part of the logging message that mentions the timeout? |
The exception that occurs is:
|
… and the solution (on the user's end) is to set the
|
Now I triage timeouts, SSL errors, and unknown errors. It seemed reasonable to add some explanation to these cases |
@larsoner Works! :) |
* upstream/main: (35 commits) [DOC] Add documentation for setting montage order (mne-tools#12160) Fix inferring fiducials from EEGLAB (mne-tools#12165) Try to fix ICA Report (mne-tools#12167) BUG: Fix bug with Report.add_ica component number (mne-tools#12156) MAINT: Add rstcheck to CIs and pre-commit (mne-tools#12163) DOC: fix sphinx style typos (mne-tools#12161) MAINT: Fix linkcheck (mne-tools#12162) ENH: Add multiple label support to source_band_induced_power, source_induced_power (mne-tools#12026) Allow automated metadata generation to be bounded by "row events" instead of explicit time windows (mne-tools#12118) ENH: Collapse only in doc gen (mne-tools#12145) [pre-commit.ci] pre-commit autoupdate (mne-tools#12155) BUG: Fix bug with interior points not showing (mne-tools#12148) ENH: Warn about versions in sys_info (mne-tools#12146) Fix in conftest.py (mne-tools#12150) ENH: set color for bad channel with spatial_colors=True (mne-tools#12142) DOC: Better documentation of realign_raw (mne-tools#12135) Add mne-icalabel wildcard (mne-tools#12143) Remove LGTM.com configuration file (mne-tools#12139) DOC: Fix typo found by codespell (mne-tools#12140) DOC: Document governance updates (mne-tools#12133) ...
* upstream/main: (26 commits) FIX: Fix bug with coreg scalars (mne-tools#12164) Changed casting rule in np.clip to allow reading of raw GDF files (mne-tools#12168) [DOC] Add documentation for setting montage order (mne-tools#12160) Fix inferring fiducials from EEGLAB (mne-tools#12165) Try to fix ICA Report (mne-tools#12167) BUG: Fix bug with Report.add_ica component number (mne-tools#12156) MAINT: Add rstcheck to CIs and pre-commit (mne-tools#12163) DOC: fix sphinx style typos (mne-tools#12161) MAINT: Fix linkcheck (mne-tools#12162) ENH: Add multiple label support to source_band_induced_power, source_induced_power (mne-tools#12026) Allow automated metadata generation to be bounded by "row events" instead of explicit time windows (mne-tools#12118) ENH: Collapse only in doc gen (mne-tools#12145) [pre-commit.ci] pre-commit autoupdate (mne-tools#12155) BUG: Fix bug with interior points not showing (mne-tools#12148) ENH: Warn about versions in sys_info (mne-tools#12146) Fix in conftest.py (mne-tools#12150) ENH: set color for bad channel with spatial_colors=True (mne-tools#12142) DOC: Better documentation of realign_raw (mne-tools#12135) Add mne-icalabel wildcard (mne-tools#12143) Remove LGTM.com configuration file (mne-tools#12139) ...
Co-authored-by: Daniel McCloy <dan@mccloy.info>
This is the best formatting I could find:
mne
version is reported, andWith this change I get:
On dev
On my machine I get:
On stable
Hacking the code to say I'm on stable the above changes to:
Outdated
Hacking the code to fake the latest release to be 1.7 we see the two lines change:
Timeout (default 2s)
If I hack the timeout to be tiny enough that it fails for my system, same
Todo
Just waiting for feedback before finishing:
Closes #12132