-
Notifications
You must be signed in to change notification settings - Fork 55
Refactor of labscript.py #102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor of labscript.py #102
Conversation
…classes, and updating code to take advantage of more modern Python (3.6+) features.
…bleDevice`, `IntermediateDevice`, etc.) to more modern Python and improved some formatting. Also fixed a bug in an error message.
…ons out of labscript.py.
…ting. Also fixed a couple of bugs with error messages (not being raised, having incorrect text, etc.)
* `config` also refactored to be inside `compiler`. * constants moved into their own file. * warnings context managers moved into the `utils` file
…raised (message was formed, but not raised), hence why the incorrect condition was never noticed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this Phil, it is sorely needed! I've made a minor comment just to confirm a minor change was intentional. Otherwise, most of the work is formatting changes and moving things which should be transparent. I can run it against an experiment in the lab (next week since we are having a snow day today), though that experiment isn't really all that complex so I doubt it would catch any edge cases that have been introduced.
I've also added another comment about removing the pylab star import. Probably scope creep for this PR, but I would be interested in addressing it in the next one.
Speaking of scope creep, what are you thoughts about adding type hints to API portions of labscript (like core labscript here)? Cost/benefit analysis makes it tight. It's a big lift that can rot quickly when interacting with external libs. Another project I'm on uses them as another layer of documentation and we occasionally manually check them with mypy to catch bugs which is a lower barrier to entry. I know the biggest problem is Device name passing. I'm pretty sure modern python has a reasonable path to not require it (structurally at least). But it may be hard to implement without breaking changes to how labscript scripts are currently done. Anyway, type hinting the API portions of labscript is a pretty decent quality of life improvement (given modern editor auto-completions) that I would interested in pursuing in the near-ish term. PS. I know this came up recently somewhere, but I can't find that conversation ATM. |
Note to future me: we'll need to update the docs to point to all these new files before final merging. |
I am keen to get this in too, although I'd prefer to have it separate to this PR. The smaller the PRs, the easier it is to bisect the changes is something goes wrong and a major bug rears it's head. At least until we get some real testing going :) |
I've updated the documentation and also fixed some other things up while I was there. There is one small issue on the API table of contents page, which seems to have pulled a docstring for the compiler class into the module list as a description, and then when you visit the |
Well, module descriptions are supposed to be pulled from the docstring at the top of the module file (before imports, after comments if present). Not required but probably helpful here to clearly state the organizing principle. I'll have to take a look at the compiler stuff tomorrow. It is probably some issue with the templating that shouldn't be hard for me to track down. |
…ocated `print_time()` function to utils.
…that breaks documentation.
Looks like it is a bug in sphinx, see sphinx-doc/sphinx#11362 . I think I'll just remove that module from the docs - it's very internal anyway. I've added module level docstrings to the other module, and the copyright header (although really that's feeling a bit pointless these days given the license file in the repo root is sufficient). And I moved a utility function I found. Hopefully this is in a better state now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good for me. I still need to actually try compiling some shots in lab, but I will actually do that this week. Once that is done, I say we go ahead and merge.
Speaking of random sphinx syntax things, if you wanted you can add a lot more info to the module-level docstrings without breaking the summary strings. Basic syntax (of all sphinx docstrings with autosummary) is to have the first line until a linebreak be the summary, then the whole docstring is shown on the associated docs page as usual. As a trivial example, the
The first line would be the summary string in the table. The entire thing would show up on the module docs page (with the linebreak). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized that I forgot to report back about compiling an experiment in the lab. Everything worked without issue. Not the most complicated experiment to compile, but it uses a Pulseblaster, NI DAQs, and a bunch of custom devices. I say this is ready to merge.
@philipstarkey Unless you have anything else you'd like to add to this PR, I'll plan on merging tomorrow. |
commit 6c5181f2fa45b572c0366285bd9a09f5b363efc2 Author: David Meyer <[email protected]> Date: Fri Apr 11 13:59:24 2025 -0400 Merge pull request #113 from dihm/workflow_refresh Workflow refresh commit 58c124a622e5db65d4bff3c2e03f83365e9bbef2 Author: David Meyer <[email protected]> Date: Fri Jul 12 17:15:52 2024 -0400 Fix typo in metadata commit 24c5b7a6af994e4ddd494fc2a6802fcb6ce1b0da Author: David Meyer <[email protected]> Date: Fri Jul 12 17:11:01 2024 -0400 Merge pull request #110 from dihm/setuptools_scm_fix2 Add setuptools_scm build arguments to pyproject.toml commit 58505653fcdb1da50c1fb0dc3f6c7bd60029068e Author: David Meyer <[email protected]> Date: Tue Mar 19 10:27:24 2024 -0400 Merge pull request #111 from dihm/runmanager_dep_break Remove latent dependency on runmanager due to `get_shot_globals`. commit 4cd0cf319ec1a3a1ba7a8f4b53f0cd48c1030b92 Author: Phil Starkey <[email protected]> Date: Sat Feb 17 15:13:13 2024 +1100 Merge pull request #102 from philipstarkey/philipstarkey/v3-refactor This is a first pass attempt at breaking up `labscript.py` into separate files. The idea here is that, while this might not be how we ultimately want things to be split up, at this point something is better than nothing. This should provide enough of a base that other people begin to feel comfortable moving things around and/or breaking things up logically. It also introduces some conceptual boundaries between output classes and the base device classes that handle timing/clock generation. My hope is that this split will open up the possibility of actually writing tests for some of this stuff. That's probably still a long way off (with several more refactors in between) but it's a step towards that goal! Other changes: * More modern context managers for warning suppression that can actually be used to enable/disable warnings, not just disable. * `config` is moved into `compiler` which means it should get reset between labscript shots and not just globally changed until you reload the compiler subprocess in runmanager. * Some better/more consistent formatting. I didn't run `black`/`ruff` but I think we should think about it soon. * Updated error message string formatting to use `f` strings for increased code readability * Fixed a mistake with trigger error detection that was never raised (and also the `if` condition around the unraised error was wrong anyway) * Fixed a minor bug in an error message for shutters (had open/close the wrong way around) * Probably some other small changes. commit 87cab030f05562171984f3ad1470678a8ac90698 Author: David Meyer <[email protected]> Date: Fri Feb 9 13:24:34 2024 -0500 Merge pull request #107 from dihm/update_workflow Update workflow pins to use node.js=20 commit 719460ec1936dc62942c4d85551c528941f7b789 Author: David Meyer <[email protected]> Date: Fri Feb 9 13:24:20 2024 -0500 Merge pull request #106 from dihm/setuptools_scm_fix Ensure setuptools_scm uses `release-branch-semver` commit 3981efc6ce62a1678a9a853073b59d702e931ff1 Author: David Meyer <[email protected]> Date: Thu Jan 18 20:55:11 2024 -0500 Merge pull request #103 from dihm/rtd_build Modernize RTD build
commit 41d6e1c Merge: bfc65fe b3869b7 Author: David Meyer <[email protected]> Date: Fri Apr 11 13:59:24 2025 -0400 Merge pull request #113 from dihm/workflow_refresh Workflow refresh commit b3869b7 Author: David Meyer <[email protected]> Date: Fri Apr 11 13:54:15 2025 -0400 Update project metadata and remove old dependencies commit 98be5f3 Author: David Meyer <[email protected]> Date: Fri Apr 11 13:54:02 2025 -0400 Update workflow to latest and greatest commit bfc65fe Author: David Meyer <[email protected]> Date: Fri Jul 12 17:15:52 2024 -0400 Fix typo in metadata commit 346ee1d Merge: 24f2d6a b3358c9 Author: David Meyer <[email protected]> Date: Fri Jul 12 17:11:01 2024 -0400 Merge pull request #110 from dihm/setuptools_scm_fix2 Add setuptools_scm build arguments to pyproject.toml commit b3358c9 Author: David Meyer <[email protected]> Date: Thu Feb 29 16:24:39 2024 -0500 Make editable installs use `pyproject.toml` setuptools_scm config. Removes no longer necessary `setup.py`. commit 225bbee Author: David Meyer <[email protected]> Date: Thu Apr 4 13:57:59 2024 -0400 Move all project metadata to `pyproject.toml` commit f41d956 Author: David Meyer <[email protected]> Date: Thu Feb 29 20:30:15 2024 -0500 Make `setuptools_scm` optional, even if installed in editable mode. commit 67a5a89 Author: David Meyer <[email protected]> Date: Thu Feb 29 16:19:46 2024 -0500 Remove environment checks in `setup.py` for setuptools_scm commit 003ce6c Author: David Meyer <[email protected]> Date: Thu Feb 29 16:19:14 2024 -0500 Remove environment checks on `__version__.py` setuptools_scm commit dfb3b00 Author: David Meyer <[email protected]> Date: Fri Feb 23 15:16:30 2024 -0500 Remove `SCM_LOCAL_SCHEME` environment variable in release workflow as it is no longer used. commit 06b0b44 Author: David Meyer <[email protected]> Date: Fri Feb 23 13:47:37 2024 -0500 Add setuptools_scm build arguments to pyproject.toml commit 24f2d6a Merge: 37be1bf 554968f Author: David Meyer <[email protected]> Date: Tue Mar 19 10:27:24 2024 -0400 Merge pull request #111 from dihm/runmanager_dep_break Remove latent dependency on runmanager due to `get_shot_globals`. commit 554968f Author: David Meyer <[email protected]> Date: Tue Mar 19 10:22:34 2024 -0400 Remove latent dependency on runmanager due to `get_shot_globals`. Bumps required version of `labscript_utils` to match. commit 37be1bf Merge: 332c180 714d982 Author: Phil Starkey <[email protected]> Date: Sat Feb 17 15:13:13 2024 +1100 Merge pull request #102 from philipstarkey/philipstarkey/v3-refactor This is a first pass attempt at breaking up `labscript.py` into separate files. The idea here is that, while this might not be how we ultimately want things to be split up, at this point something is better than nothing. This should provide enough of a base that other people begin to feel comfortable moving things around and/or breaking things up logically. It also introduces some conceptual boundaries between output classes and the base device classes that handle timing/clock generation. My hope is that this split will open up the possibility of actually writing tests for some of this stuff. That's probably still a long way off (with several more refactors in between) but it's a step towards that goal! Other changes: * More modern context managers for warning suppression that can actually be used to enable/disable warnings, not just disable. * `config` is moved into `compiler` which means it should get reset between labscript shots and not just globally changed until you reload the compiler subprocess in runmanager. * Some better/more consistent formatting. I didn't run `black`/`ruff` but I think we should think about it soon. * Updated error message string formatting to use `f` strings for increased code readability * Fixed a mistake with trigger error detection that was never raised (and also the `if` condition around the unraised error was wrong anyway) * Fixed a minor bug in an error message for shutters (had open/close the wrong way around) * Probably some other small changes. commit 332c180 Merge: a99573e 1071fee Author: David Meyer <[email protected]> Date: Fri Feb 9 13:24:34 2024 -0500 Merge pull request #107 from dihm/update_workflow Update workflow pins to use node.js=20 commit a99573e Merge: 6f38c9a bf0d2b7 Author: David Meyer <[email protected]> Date: Fri Feb 9 13:24:20 2024 -0500 Merge pull request #106 from dihm/setuptools_scm_fix Ensure setuptools_scm uses `release-branch-semver` commit bf0d2b7 Author: David Meyer <[email protected]> Date: Fri Feb 9 13:21:32 2024 -0500 Ensure setuptools_scm uses `release-branch-semver` Updates setuptools and setuptools_scm pin in the build. commit 1071fee Author: David Meyer <[email protected]> Date: Fri Feb 9 13:19:20 2024 -0500 Update workflow pins to use node.js=20 commit 714d982 Author: philipstarkey <[email protected]> Date: Sat Jan 27 16:24:56 2024 +1100 Remove `compiler` module from autosummary documentation due to a bug that breaks documentation. commit 7e0ab10 Author: philipstarkey <[email protected]> Date: Sat Jan 27 16:20:40 2024 +1100 Reamed `Compiler` class to work around an autosummary bug commit 32b3d6e Author: philipstarkey <[email protected]> Date: Sat Jan 27 16:15:18 2024 +1100 Updated module doc strings so they are only a single sentence and relocated `print_time()` function to utils. commit 5ea15e8 Author: philipstarkey <[email protected]> Date: Sat Jan 27 16:05:28 2024 +1100 Added copyright notices and docstrings to new files commit 5c04f42 Author: philipstarkey <[email protected]> Date: Sat Jan 27 15:49:00 2024 +1100 Split `AnalogIn` into it's own module so that documentation makes a little more sese commit 59757a4 Author: philipstarkey <[email protected]> Date: Sat Jan 27 13:08:04 2024 +1100 Added note about not using generic classes directly commit 9a9e205 Author: philipstarkey <[email protected]> Date: Sat Jan 27 13:00:34 2024 +1100 Attempt to fix module cross reference commit 8775e24 Author: philipstarkey <[email protected]> Date: Sat Jan 27 12:55:17 2024 +1100 Updated connection table documentation with links to class documentation. commit 36dad35 Author: philipstarkey <[email protected]> Date: Sat Jan 27 12:37:53 2024 +1100 First pass at getting new files included in the docs commit ba082c1 Merge: 91025be 6f38c9a Author: Phil Starkey <[email protected]> Date: Sat Jan 27 12:24:42 2024 +1100 Merge branch 'labscript-suite:master' into philipstarkey/v3-refactor commit 91025be Author: philipstarkey <[email protected]> Date: Sat Jan 27 12:16:47 2024 +1100 Revert change to use public property in `ClockLine.add_device` internals commit 6f38c9a Merge: 01cc205 dd410e4 Author: David Meyer <[email protected]> Date: Thu Jan 18 20:55:11 2024 -0500 Merge pull request #103 from dihm/rtd_build Modernize RTD build commit dd410e4 Author: David Meyer <[email protected]> Date: Thu Jan 18 20:51:16 2024 -0500 Modernize RTD build commit 5b580ad Author: philipstarkey <[email protected]> Date: Thu Jan 4 17:30:11 2024 +1100 Fix bug with cached imports in utils commit 4d0089b Author: philipstarkey <[email protected]> Date: Thu Jan 4 17:29:52 2024 +1100 Fix bug in error condition. This was an error that perviously wasn't raised (message was formed, but not raised), hence why the incorrect condition was never noticed. commit 96b3dd3 Author: philipstarkey <[email protected]> Date: Thu Jan 4 17:28:26 2024 +1100 Revert change made that tried to write to a property commit 6d4535b Author: philipstarkey <[email protected]> Date: Wed Jan 3 20:40:40 2024 +1100 * Move `Output` classes to a separate file. * `config` also refactored to be inside `compiler`. * constants moved into their own file. * warnings context managers moved into the `utils` file commit 63ce813 Author: philipstarkey <[email protected]> Date: Wed Jan 3 19:59:13 2024 +1100 Updated output classes to more modern Python and improved some formatting. Also fixed a couple of bugs with error messages (not being raised, having incorrect text, etc.) commit 6e43872 Author: philipstarkey <[email protected]> Date: Wed Jan 3 16:31:23 2024 +1100 Moved core classes into their own files. Also moved some utils functions out of labscript.py. commit 994ccc6 Author: philipstarkey <[email protected]> Date: Wed Jan 3 16:14:57 2024 +1100 Updated core device subclasses (`Pseudoclock`, `ClockLine`, `TriggerableDevice`, `IntermediateDevice`, etc.) to more modern Python and improved some formatting. Also fixed a bug in an error message. commit 461344e Author: philipstarkey <[email protected]> Date: Sat Dec 30 21:14:30 2023 +1100 Break out `Device` class into it's own file commit 5345e6d Author: philipstarkey <[email protected]> Date: Sat Dec 30 21:11:48 2023 +1100 Initial work centered around refactoring the `Device` and `compiler` classes, and updating code to take advantage of more modern Python (3.6+) features.
This is a first pass attempt at breaking up
labscript.py
into separate files. The idea here is that, while this might not be how we ultimately want things to be split up, at this point something is better than nothing. This should provide enough of a base that other people begin to feel comfortable moving things around and/or breaking things up logically. It also introduces some conceptual boundaries between output classes and the base device classes that handle timing/clock generation. My hope is that this split will open up the possibility of actually writing tests for some of this stuff. That's probably still a long way off (with several more refactors in between) but it's a step towards that goal!Other changes:
config
is moved intocompiler
which means it should get reset between labscript shots and not just globally changed until you reload the compiler subprocess in runmanager.black
/ruff
but I think we should think about it soon.f
strings for increased code readabilityif
condition around the unraised error was wrong anyway)Suggestions for reviewing:
example.py
already (looks the same).from pylab import *
in the new files. I believe I caught every case where that would have overridden a builtin function that we relied on, and updated the code to use it directly from numpy instead, but I may have missed one or two. I almost missed the use ofdivmod
. Not sure how much this matters though. There is also a good chance that pylab is doing less overriding of inbuilt functions these days than when we initially wrote it. And since we have no unit tests, we won't have caught those historical changes over the last 13 years anyway 🤷compiler