epics-base / epicscorelibs

EPICS core libraries packaged as a "python" module
https://pypi.org/project/epicscorelibs/
Other
4 stars 10 forks source link

Incorrect warning when using epciscorelibs.path.cothread #22

Closed AlexanderWells-diamond closed 2 months ago

AlexanderWells-diamond commented 1 year ago

I believe that the warning issued in epicscorelibs.path.cothread is misleading, or possibly entirely wrong.

cothread's current CA-loading code can be found here. It is run as soon as cothread.catools is imported. In there we can see the first thing it does is check if CATOOLS_LIBCA_PATH exists in the environment, and if it does it takes it as gospel. If it does not, it will defer to epicscorelibs if it exists. Otherwise, it will fall back to trying to find the library itself.

(I will note that the epicscorelibs loading code was added a few months after the warning was introduced in this package - I suspect it was in response to the warning but then never cleaned up)

The possible scenarios are:

  1. There is no CATOOLS_LIBCA_PATH variable, cothread is imported, epicscorelibs is imported. Cothread will defer to epicscorelibs's loading mechanism, but when epicscorelibs is itself imported it will still issue a warning.
  2. There is a CATOOLS_LIBCA_PATH variable, cothread is imported, epicscorelibs is imported. Cothread will take the variable as gospel. Epicscorelibs will issue the warning, and also take no action as the variable already exists.

If epicscorelibs is imported first, there will always be a CATOOLS_LIBCA_PATH defined by the time cothread is imported, so there is no issue there.

So I believe the fix is, at minimum, to move the warning inside of the if condition so that it is only issued if work is done, and possibly the entire warning should be removed as, as outlined in (1), there isn't a problem (except possibly attempting to load the library twice, but I suspect that's a no-op asides from some wasted CPU cycles)

cc @Araneidae

mdavidsaver commented 1 year ago

believe that the warning issued in epicscorelibs.path.cothread is misleading, or possibly entirely wrong.

A more exact warning would be something like the following. However, I see this as a distinction which does not really matter to an end user. Which leaves it as our problem.

epicscorelibs.path.cothread must be imported before cothread.catools has loaded libca to have effect.

So a technically more precise test would be to detect if libca has actually been loaded, instead of relying on 'cothread.load_ca' in sys.modules as a proxy condition. I know this is possible using OS specific interfaces, however I do not know offhand if ctypes abstracts this. Worth investigating.

Also, I would like to continue to issue this warning when older versions of cothread are involved.

AlexanderWells-diamond commented 1 year ago

So a technically more precise test would be to detect if libca has actually been loaded, instead of relying on 'cothread.load_ca' in sys.modules as a proxy condition. I know this is possible using OS specific interfaces, however I do not know offhand if ctypes abstracts this. Worth investigating.

Some investigation has not revealed an abstraction in the ctypes layer for this. I suspect it would have to be implemented per-platform.

I take your point about old cothread versions. Looking at the history, the "safe" cothread version is 2-16. Is it sufficient to inspect cothread's version in cothread.version? It's made a little tricky as it is just a string, '2.16.0', but it would be doable.

mdavidsaver commented 1 year ago

Some investigation has not revealed an abstraction in the ctypes layer for this.

Agreed. I don't think at this point that it is worth the effort to do pursue this.

It's made a little tricky as it is just a string, '2.16.0'

Maybe a case for adding a tuple? (as sys.version and sys.version_info)

mdavidsaver commented 1 year ago

At worst, cothread could filter out this warning.

import warnings
warnings.filterwarnings('ignore', module='epicscorelibs\.path\.cothread', message='.*import.*')
AlexanderWells-diamond commented 1 year ago

Maybe a case for adding a tuple? (as sys.version and sys.version_info)

A good idea, but that wouldn't help with spotting the cut-off version, or any of the versions since then, so to properly spot it in epicscorelibs we'd still have to parse the string.

At worst, cothread could filter out this warning.

A fair point. Although as you say, it's probably not the right solution but it would work.

mdavidsaver commented 1 year ago

... but that wouldn't help with spotting the cut-off version ...

Adding a tuple will bound the range of versions which are a special case. Then, assuming cothread versions have been well behaved so far, something like the following would work.

try:
    from cothread import version_info as cothread_version
except ImportError:
    from cothread import version as cothread_version
    cothread_version = tuple(int(c) for c in cothread_version.split('.'))

Or, if you would be happy simply to look forward, then add cothread.version_info and make the test something like:

if hasattr('version_info', sys.modules.get('cothread')):
    pass # newer cothread automatically falls back to epicscorelibs
elif 'cothread.load_ca' in sys.modules or 'cothread.catools' in sys.modules:
    warnings.warn("epicscorelibs.path.cothread must be imported before cothread.catools to have effect")

There would continue to be a range of versions which erroneously warn. Recommend an upgrade for anyone who complains.

AlexanderWells-diamond commented 1 year ago

Thanks. I'm in the process of refactoring cothread, and it will include a __version__ as a string and a __version_tuple__ as, you guessed it, a tuple. I'll let you know when I've made a cothread release with those changes.

AlexanderWells-diamond commented 1 year ago

I've just released cothread version 2.19.0, which includes new version information:

>>> import cothread
>>> cothread.__version__
'2.19'
>>> cothread._version.__version_tuple__
(2, 19)

Note we only directly publish the regular __version__ from the top level, but the tuple is available from the _version file.

Will this work for you?