Skip to content

Conversation

@larsoner
Copy link
Member

I think the root of the SphinxWindows problem is some case sensitivity. If this fixes everything except mne.Dipole and mne.label, the fix might be to move our doc of these classes to mne.label.Label and mne.dipole.Dipole...

@larsoner larsoner changed the title Cmds MAINT: Fix documentation building on Windows Apr 25, 2023
@drammock
Copy link
Member

I think the root of the SphinxWindows problem is some case sensitivity

OMFG AYKM?

@larsoner
Copy link
Member Author

I wish. Cost me more hours than I care to admit today to track it down 😢

@larsoner
Copy link
Member Author

Ready for review/marking for merge when green from my end. Any additional commits should just be updating intersphinx refs

@larsoner larsoner marked this pull request as ready for review April 25, 2023 22:54
Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be great if you could explain / document why these changes are needed (maybe at next dev meeting?), i.e.

  • why is mne.Dipole not OK but mne.Epochs is OK?
  • why are some source filenames being changed to have leading underscores? Is there a rule we should follow regarding this?

@larsoner
Copy link
Member Author

why is mne.Dipole not OK but mne.Epochs is OK?

That's an excellent question. I'll investigate tomorrow. Maybe something having to do with the import or documentation order would make the mne.Dipole -> mne.dipole.Dipole changes unnecessary...

why are some source filenames being changed to have leading underscores? Is there a rule we should follow regarding this?

We should have as few public namespaces as possible. I doubt anyone ever did from mne.preprocessing.xdawn import Xdawn as we document it as mne.preprocessing.Xdawn but it's of course possible they did. This PR makes the mne.preprocessing.xdawn namespace private with the leading underscore. Risk of breakage seems low enough I think.

@larsoner larsoner marked this pull request as draft April 25, 2023 23:09
@larsoner
Copy link
Member Author

Okay the Sphinx problem comes down to this:

>>> import mne
>>> mne.Dipole
<class 'mne.dipole.Dipole'>
>>> import mne.Dipole
>>> mne.Dipole
<module 'mne.Dipole' from 'C:\\Users\\larsoner\\python\\mne-python\\mne\\Dipole.py'>
# Okay, bad enough, but let's continue
>>> from mne import Dipole
>>> Dipole
<module 'mne.Dipole' from 'C:\\Users\\larsoner\\python\\mne-python\\mne\\Dipole.py'>

At some point sphinx tries import mne.Dipole which should fail (it's a class not a package) but case-insensitivity allows it to work. After this sys.modules['mne.Dipole'] exists and mne.Dipole is now broken. I've opened sphinx-doc/sphinx#11362 but I'm not super optimistic about a workaround.

I say we wait for the sphinx people to respond and if there is no workaround in the next few months then we might have to just get rid of the Windows build :(

@larsoner
Copy link
Member Author

Not super optimistic on the timeline so I'll close this for now

@larsoner larsoner closed this Apr 28, 2023
@larsoner larsoner deleted the cmds branch April 28, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants