Relocate labscript_devices import machinery

[philipstarkey]

The labscript_devices import machinery has now been relocated to labscript_utils as the first step in breaking the circular dependency between labscript-devices and blacs packages (see labscript-suite/labscript-devices#59).

The code in _import_machinery.py is equivalent to that in labscript_devices/init.py exept that __name__ has been replaced with 'labscript_devices'.

There is also some code that chooses whether to use the old labscript_devices code or the new labscript_utils code depending on the version of labscript_devices. The specific version will need to be adjusted to match whatever version removes the code from labscript_devices (and even then, would not catch the case of users with local changes in a development install) so this may not be the best approach, but I couldn't think of another way to ensure compatibility between old and new labscript-suite components without reintroducing a different circular dependency.

The labscript_devices __init__.py file can then be replaced with from labscript_utils.labscript_devices import * so as not to need to update every device class file.

[chrisjbillington]

Sweet! This seems like a good way to break the circular dependency. labscript_devices will now just be device code. I'll just try to wrap my head around this conditional import in __init__.py.

[philipstarkey]

Sweet! This seems like a good way to break the circular dependency. labscript_devices will now just be device code. I'll just try to wrap my head around this conditional import in __init__.py.

I can explain the conditional import if you like in detail of you like but the main idea is that there are 8 permutations of old/new labscript-utils, labscript-devices and BLACS (or runviewer) and this ensures they all work without attempting to build the device registry more than once.

[philipstarkey]

Sweet, I get it.

Two things:

1. labscript-devices may not be installed (after all it is not a dependency of labscript-utils!), so getting its version may fail. Either this permutation also needs to be taken into account, or (and I think this is more idiomatic) you could use the ability to import the machinery from labscript_devices as a more direct test of whether it's there, rather than basing it on the version, such that __init__.py would be something like:

try:
    from labscript_devices import ClassRegister
except ImportError:
    # labscript-devices >= 3.1, device registry now lives here:
    from ._import_machinery import *
else:
    # labscript-devices < 3.1, use device registry from there:
    from labscript_devices import *

This doesn't work. In 3.1, (with labscript_devices/__init__.py containing from labscript_utils.labscript_devices import *), importing labscript_devices after labscript_utils results in labscript_devices not sucessfully importing the import machinery. This means all of the device classes fail (and would need to be updated to use labscript_utils). I guess we could do that, but the original method did not have this failure mode (and means that user device classes outside of labscript_devices do not need to be updated - they'll currently be expecting the import machinery to come from labscript_devices). I assume something about the circular import is the issue here.

2. Another (sub)module named labscript_devices puts a bad taste in my mouth. How about device_registry or similar? There are no devices here, just a registry of them and the functions for registering them. Wouldn't mind naming _import_machinery the same as whatever you call the directory but with a leading underscore, i.e. _device_registry, but whatever.

Yeah, that's a good point. I was following the convention I made for labscript-c-extensions, but in this case the code is not just for labscript_devices, but for BLACS and runviewer as well.

[philipstarkey]

So I don't think the proposed try/except method propose will work because even though an import error occurs (because ClassRegister doesn't exist in labscript_devices), labscript_devices still gets put in sys.modules which then prevents it from importing properly later.

So I've wrapped the call to importlib_metadata in a try/except instead which seems to do the trick.

Again, the specific version in this code is dependent on changes to labscript_devices being merged and published as 3.1.0.dev16 (which should happen if it is the next PR to be merged). I'll make that PR now.

Renamed the files/folders here as requested.

[philipstarkey]

Actually I'm a little bit confused about versions. Locally my labscript_devices (tip of master) says 3.0.0b3.dev3 but test PyPI says 3.1.0.dev16? Shouldn't these be the same?

[rpanderson]

Actually I'm a little bit confused about versions. Locally my labscript_devices (tip of master) says 3.0.0b3.dev3 but test PyPI says 3.1.0.dev16? Shouldn't these be the same?

Distributed versions use the release-branch-semver version scheme of setuptools_scm. This was introduced in pypa/setuptools_scm#430 but didn't make it into the released version of setuptools_scm when we last worked on setup.py (see pypa/setuptools_scm#441). As such, we didn't want to rely on developers having the development version of setuptools_scm, so we restricted the use of this version scheme to our GitHub Actions by installing setuptools_scm from GitHub environment and setting the environment variable SCM_VERSION_SCHEME = release-branch-semver.

Now that release-branch-semver is in setuptools_scm, we should make this version scheme the default in all projects (and bump the setuptools_scm requirement to >=4.1.0).

In the meantime, you can get a local version consistent with that on Test PyPI by setting the above environment variable as described (provided you have setuptools_scm>=4.1.0).

Update: See labscript-suite/labscript-suite#53.

[rpanderson]

Locally my labscript_devices (tip of master) says 3.0.0b3.dev3

That's also a little odd, as even with SCM_VERSION_SCHEME=guess-next-dev (what setup.py currently uses if you don't have the environment variable set), the latest tagged commit on master is labscript-suite/labscript-devices@535c9b305a273965b946d23bee96ba57d8c1c48a, which is v3.0.0rc1 (newer than the v3.0.0b2 which your local versioning seems to be based off).

Actually, you are likely reporting the version importlib.metadata is reporting. If you want this to be correct, you'll also need to pip install -e instead need to read the __version__ attribute of any packages you want consistent versioning of.

Edited to redact advice about running pip install -e on developer installations.

[philipstarkey]

Locally my labscript_devices (tip of master) says 3.0.0b3.dev3

That's also a little odd, as even with SCM_VERSION_SCHEME=guess-next-dev (what setup.py currently uses if you don't have the environment variable set), the latest tagged commit on master is labscript-suite/labscript-devices@535c9b3, which is v3.0.0rc1 (newer than the v3.0.0b2 which your local versioning seems to be based off).

Running pip install -e . again (on labscript_devices) has changed the version I now see (to 3.0.0rc2.dev15 when on master) but I thought we didn't need to do that? Particularly because doing that resulted in it installing blacs, labscript and labscript_utils from PyPI in order to meet the requirements in setup.cfg of labscript_devices, because those are not reporting their correct versions without running pip install -e again. I'll now have to go and redo the editable installs for those using --no-deps to repair my editable labscript install :(

EDIT: Actually we would never expect the editable installs to report major version numbers because these will be in the maintenance branch...this could be a problem for upgrading editable installs.

[rpanderson]

It depends. We don't use importlib.metadata for developer installations to get the __version__ attribute. See __version__.py. Perhaps you should get the version from that attribute instead of using importlib.metadata.

[rpanderson]

doing that resulted in it installing blacs, labscript and labscript_utils from PyPI in order to meet the requirements in setup.cfg of labscript_devices, because those are not reporting their correct versions without running pip install -e again. I'll now have to go and redo the editable installs for those using --no-deps to repair my editable labscript install :(

Sorry! This wasn't the case a few days ago (before we bumped versions in setup.cfg). My advice should have been to source the version from the __version__ attribute rather than using importlib.metadata. I'll redact my advice above so as not to lead anyone else down the garden path.

[philipstarkey]

It depends. We don't use importlib.metadata for developer installations to get the __version__ attribute. See __version__.py. Perhaps you should get the version from that attribute instead of using importlib.metadata.

If I do that then new labscript_devices cannot import the import machinery from labscript utils due to the circular import (that for some reason does not populate labscript_devices with the contents of labscript_utils.device_registry, presumably because that import has not complete yet)

[philipstarkey]

So importlib_metadata doesn't work for this PR (due to editable install issue above) and neither does circular imports out of the box. I guess I could do the circular import and then monkey patch in the import machinery into labscript_devices from labscript_utils? That's a bit hacky though...

@chrisjbillington do you have any better ideas?

[rpanderson]

• Would duplicating the functionality of __version__.py work? (Rather than importing the module and using its __version__ attribute.) i.e. Use importlib.metadata.version() for regular installations, and setuptools_scm.get_version() for editable installations?

• Would your conclusions be the same if the changes of labscript-suite/labscript-suite#53 were adopted suite-wide?

[philipstarkey]

• Would duplicating the functionality of __version__.py work? (Rather than importing the module and using its __version__ attribute.) i.e. Use importlib.metadata.version() for regular installations, and setuptools_scm.get_version() for editable installations?

That's a good idea. I'd need to use inportlib_metadata to get the path to the module, but I've done that before.

• Would your conclusions be the same if the changes of labscript-suite/labscript-suite#53 were adopted suite-wide?

I don't think it changes anything because it's not just the different scheme, but that fact that importlib_metadata version is fixed at install time, even for editable installs, that breaks my code. But your above suggestion should work around that. There will still be se rare combinations of test PyPI installs and editable install versions that might not play nice with this PR, but that's impossible to fix I think due to the historical use of the older setuptools_scm version.

[philipstarkey]

P.S. I'm not going to be able to take a look at this for a few days so you are both welcome to push appropriate changes if you want to try something sooner

[chrisjbillington]

Yes, I think I have a better idea. The issue with my earlier suggestion is the fact that there is a circular import and new labscript_devices cannot import the names from labscript_utils.device_registry, as they are not yet defined since it is still initialising.

A solution is to define them ahead of time, but then throw them out and replace them with the ones from labscript_devices if it turns out labscript_devices is old:

from ._device_registry import *

# Backwards compatibility for labscript-devices < 3.1. If labscript_devices defines the
# device registry as well, undo the above import and use the contents of
# labscript_devices instead. The above import must be done first so that the names are
# available to labscript_devices during the below import, since as of 3.1 it imports
# this module as well.
try:
    from labscript_devices import ClassRegister
    if ClassRegister.__module__ == 'labscript_devices':
        for name in _device_registry.__all__:
            del globals()[name]
        from labscript_devices import *
except ImportError:
    pass

for this to work we should define an __all__ in _device_registry that lists the public API, otherwise the 'undoing the import' code will need to inspect which names start with an underscore, which is what import * does in the absence of an __all__.

[chrisjbillington]

I've pushed my suggested change. Tested and it appears to work.

Even if you prefer an explicit version check, the same idea of unconditionally importing the new module would work to allow you to import labscript_devices and look at its __version__ attribute, before potentially rolling back the import of the new module.

For what it's worth, the fact that we can't relocate a module trivially and have had to think hard about this is a point against having separate repos. I'm not saying we shouldn't have separate repos, but just pointing out that this is one of the downsides of the choice we made. It's usually not as bad as this though.

[chrisjbillington]

Going to be bold and merge this and the corresponding labscript_devices PR.