add version string

[dominikwelke]

add __version__ info

see mne-tools/mne-installers#349

[dominikwelke]

hi @drammock @larsoner

PR pretty self explanatory, but there is a more general discussion:
how do we want to handle diversions of this package from the base curry-python-reader repository?

e.g.

1. there's a bugfix PR (see improve impedance readout #2) which i also suggested upstream weeks ago, with no reaction.
2. tkinter creates problems with some of our CIs. im sure we can account for that on our side, but i guess i could also just tweak the import in the curryreader package to only happen when needed (i.e. not in our io case). imo this would improve the thing for some other users, too.

[larsoner]

how do we want to handle diversions of this package from the base curry-python-reader repository?

If they don't merge fixes, we can. And whatever their version number is, we can release with a .post1, .post2 etc. or whatever. That way we know if there is a .post then it's not the vanilla upstream version, but some variant of ours, but otherwise meant to have the same functionality.

It's possible there is some other semantic versioning name other than post that would work, too -- feel free to work. I wouldn't go with dev because that's meant for dev versions and it's widely used, which isn't the meaning we'd be trying to convey. post is probably also a misnomer but I don't see it used very much so maybe it's okay to mis-use it this way 😄

[drammock]

whatever their version number is, we can release with a .post1, .post2 etc.
It's possible there is some other semantic versioning name other than post that would work, too

The version specifier spec is [N!]N(.N)*[{a|b|rc}N][.postN][.devN] so I think (ab)using .postN for this purpose is probably the only option (and actually a pretty clever idea).

tkinter creates problems with some of our CIs.

I was afraid we'd start hitting problems like this. Ideally we figure out a minimal way to purge functionality we don't need, so that rebasing on top of upstream changes (if/when they ever happen) will be minimally painful. What is the problem tkinter is causing?

[dominikwelke]

The version specifier spec is [N!]N(.N)*[{a|b|rc}N][.postN][.devN] so I think (ab)using .postN for this purpose is probably the only option (and actually a pretty clever idea).

sounds good.
but i think we're ahead of them already. you gave the current packaged version 0.1.0 while the upstream version is 0.0.1

[larsoner]

Whoops! Okay maybe we just decouple the versioning entirely then...

[dominikwelke]

What is the problem tkinter is causing?

the linux pipelines dont have it (e.g.
https://app.circleci.com/pipelines/github/mne-tools/mne-python/27481/workflows/d02db59c-6e83-4a63-9654-24d0b516164a/jobs/73261 )
we had a similar problem in the conda-forge recipe PR.

[larsoner]

Ugh yes we should nest any tkinter imports ASAP I think

[drammock]

so, removing it from the dependencies list would be enough? I think we talked about not running their built-in tests, but if we're now talking about fixing bugs too, I feel like we probably should actually run the tests on our repo's CIs too... in which case purging tkinter from the dependencies will not help. 🤔

[larsoner]

The CI error is:

      File "/home/circleci/project/mne/io/curry/curry.py", line 10, in <module>
        import curryreader
      File "/home/circleci/python_env/lib/python3.12/site-packages/curryreader.py", line 5, in <module>
        import tkinter as tk
    ModuleNotFoundError: No module named 'tkinter'

so the problem is you can't import curryreader at all without tkinter installed, even if you plan on doing nothing with a GUI. Hence the idea to nest the tkinter import in curryreader to make it an optional rather than (currently) required dependency

[dominikwelke]

yes, this was what i suggested to do

[dominikwelke]

do you have any deeper test of all dependencies in mne that would cause probems in later checks?

[larsoner]

I'm not quite sure what you mean... I think MNE fails there because we've mis-specified our dependencies here. tkinter is required to import curryreader, so when it happens in CIs it fails. Once our (hard) dependencies in specified here match the actual behavior -- i.e., once tkinter isn't required to import curryreader -- then MNE should be fine, and the implicit pip install curryreader that happens should make it so things work fine.

[dominikwelke]

I'm not quite sure what you mean

i meant a test that checks all dependencies from the requirement files rather than whats actually loaded when running mne.
not sure if this were even possible or meaningful :D

but nevermind - i just saw that tkinter is not even an explicit requirement in the curryreader package. makes sense, otherwise it shouldnt have complained. - just as you wrote

[dominikwelke]

missing __version__ and tkinter issues are addressed.

remains the question with the actual bugfixes.

[larsoner]

My vote is to merge this and #2 and then cut a release. @drammock ?

[dominikwelke]

p.s. - the nesting i commited earlier is the minimal change but not optimal, as users loose the info what caused the issue. last commit is a more informative version.

feel free to ignore

[dominikwelke]

btw, i blindly mimicked the __version__ specification from base mne.
any chance that the implicit import of importlib would fail giving users the wrong version?

[larsoner]

Before pushing a release to conda-forge we should check. Then in conda-forge I usually have checks like this

https://github.com/conda-forge/mne-qt-browser-feedstock/blob/31aa5ec933153baf76ca2ae1e175706afb6f9944/recipe/meta.yaml#L46

[drammock]

My vote is to merge this and #2 and then cut a release. @drammock ?

SGTM