fix(api): ignore missing aionotify more properly

[mrakitin]

Overview

The aionotify is supposed to work only under Linux (that's why the corresponding conda-forge package exists for Linux only). The failure with the other platforms was observed during the work on conda-forge/staged-recipes#15705 (failed build log for OSX can be found here).

Changelog

Added an additional ModuleNotFoundError exception to catch the missing package for OSX and Windows platforms properly. Thanks to https://stackoverflow.com/a/6470452 for the hints.

Review requests

Please review and accept the addition as it fixes the missing import.

Risk assessment

The risk is minimal in my opinion.

[SyntaxColoring]

Thanks so much for contributing this fix!

This looks basically right to us. We're taking a closer look because we're not sure what the original except OSError is doing, nor why the missing except ModuleNotFoundError never caused problems for us internally.

[mrakitin]

Thanks for the prompt reply, @SyntaxColoring! Please take your time to review and check it. On my side, a patch based on my suggestions here in this PR worked for packaging on OSX/Windows.

To give a bit of background, the package is used at the NSLS-II LiX beamline (see https://github.com/NSLS-II-LIX/lixtools/blob/main/lixtools/ot2.py). We at NSLS-II heavily rely on the conda packages for our deployments. I am working on packaging the opentrons package with conda, and the PR to do it is ready for review by the conda-forge team (see conda-forge/staged-recipes#15705). Once it's merged, the package will be available to install from https://anaconda.org/conda-forge/. You and your team are welcome to co-maintain the feedstock/package for conda, please let me know if you wish to become a maintainer there.

I noticed that the opentrons package exists at https://anaconda.org/opentrons/opentrons, however, a quite old version 2, and I wasn't able to find the corresponding recipe meta.yaml file. Do you happen to have one as an example?

[SyntaxColoring]

Requesting one small change, and then this looks good to me. Oops looks like you've already committed it. Thanks! Just waiting for our CI to pass, and then I'll merge. ✅

---

Notes on why this PR makes sense:

If you install aionotify on a non-Linux platform, import aionotfy will raise an OSError. We appear to unconditionally install aionotify even on platforms where it won't work, so that's why the original except OSError was good enough for our dev environments and installations of our official PyPI package.

As @mrakitin explained, the package for aionotify in Conda takes a different approach: a package that doesn't work on a platform should just not be installable on that platform in the first place. (This seems sensible to me, and I wonder if we could do something similar for our official PyPI package.) So, an additional except ModuleNotFoundError is needed.

---

Also—and this is a matter for a separate pull request—I think it would be nice if we refactored this so we don't have to care about this platform dependence at so many import sites. We could either have a helper module like opentrons.aionotify that encapsulates the check, or we could add a property like .notifications_supported to something in opentrons.hardware_controller.

[codecov]

Codecov Report

Merging #8154 (5bc890e) into edge (33e3ee3) will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge    #8154      +/-   ##
==========================================
+ Coverage   86.92%   87.06%   +0.14%     
==========================================
  Files         413      414       +1     
  Lines       22106    22209     +103     
==========================================
+ Hits        19216    19337     +121     
+ Misses       2890     2872      -18     

Impacted Files	Coverage Δ
opentrons/hardware_control/emulation/app.py	86.66% <0.00%> (-5.34%)	⬇️
...ode_parsing/g_code_program/supported_text_modes.py	97.36% <0.00%> (-2.64%)	⬇️
opentrons/hardware_control/emulation/smoothie.py	98.71% <0.00%> (-1.29%)	⬇️
...pentrons/hardware_control/g_code_parsing/g_code.py	94.52% <0.00%> (ø)
...hardware_control/g_code_parsing/protocol_runner.py	100.00% <0.00%> (ø)
opentrons/hardware_control/emulation/tempdeck.py	97.29% <0.00%> (+0.15%)	⬆️
...entrons/hardware_control/emulation/thermocycler.py	93.42% <0.00%> (+0.17%)	⬆️
opentrons/hardware_control/api.py	84.70% <0.00%> (+0.38%)	⬆️
opentrons/hardware_control/emulation/magdeck.py	92.10% <0.00%> (+0.43%)	⬆️
opentrons/hardware_control/controller.py	80.89% <0.00%> (+0.56%)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33e3ee3...5bc890e. Read the comment docs.

[SyntaxColoring]

To give a bit of background, the package is used at the NSLS-II LiX beamline

Man, that's cool.

You and your team are welcome to co-maintain the feedstock/package for conda, please let me know if you wish to become a maintainer there.

Thanks for the offer! My hunch is that we'll want to focus on maintaining our official PyPI package, but I'll ask internally to see if we can help out with Conda, too.

I noticed that the opentrons package exists at https://anaconda.org/opentrons/opentrons, however, a quite old version 2, and I wasn't able to find the corresponding recipe meta.yaml file. Do you happen to have one as an example?

Hm, that's interesting. It looks like that was uploaded before my time at Opentrons. I'll ask internally if anyone knows what the deal is. Otherwise, I don't think we can be very helpful, since we're generally uninvolved in and unfamiliar with the Conda ecosystem today—sorry. 🙁

[Update] Yeah, we're not sure what that Conda package is. It's very old—possibly, it's the OT-One API.

[mrakitin]

If you install aionotify on a non-Linux platform, import aionotfy will raise an OSError. We appear to unconditionally install aionotify even on platforms where it won't work, so that's why the original except OSError was good enough for our dev environments and installations of our official PyPI package.

@SyntaxColoring, thanks for the quick review and additional details on why OSError was needed, that makes sense to me.

This seems sensible to me, and I wonder if we could do something similar for our official PyPI package.

That should be possible. https://stackoverflow.com/a/29222444 and the answers below suggest solutions for different platforms.

Also—and this is a matter for a separate pull request—I think it would be nice if we refactored this so we don't have to care about this platform dependence at so many import sites. We could either have a helper module like opentrons.aionotify that encapsulates the check, or we could add a property like .notifications_supported to something in opentrons.hardware_controller.

I like this idea a lot!

Thanks for the offer! My hunch is that we'll want to focus on maintaining our official PyPI package, but I'll ask internally to see if we can help out with Conda, too.

Not a problem. I will be maintaining the conda-forge feedstock, so in case anyone is willing to join, they are welcome.

Regarding PyPI: do you plan to also distribute the sdist (.tar.gz) files? I noticed only .whl packages are there now. I tried to generate the sdist for api and shared-data/python with python setup.py sdist, but there was a number of errors due to missing .json files, etc. I wonder if it's supported at all. For the conda recipe, I used the commands from your Dockerfile, which seem to work OK.

[SyntaxColoring]

Regarding PyPI: do you plan to also distribute the sdist (.tar.gz) files?

No current plans, but I think we'd be open to it if there's some need that .whls don't fulfill? If that's the case, feel free to open a feature request for sdists.