labscript-suite / labscript-devices

A modular and extensible plugin architecture to control experiment hardware using the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦.
http://labscriptsuite.org
Other
5 stars 58 forks source link

Initial labscript-devices docs #70

Closed dihm closed 3 years ago

dihm commented 3 years ago

With a few of the easier packages down, I'm going to move on to something harder but very necessary.

Right now I only have a very rough outline of the documentation structure, and I intend to fill in quite a few holes along the way since I am generally more familiar with the details here.

Before I get too deep, I wanted to get input on a style preference: should we aim to have the bulk of the documentation as docstrings (up to and including module level stuff like basic usage for each device) or only have docstrings for API level documentation of classes and functions, writing the higher level stuff out separately in docs files?

philipstarkey commented 3 years ago

I think comprehensive docstrings would be the place to start. I think regardless of higher level documentation, those need to be done. However I don't think they need to tell a coherent story necessarily. That's where future higher level docs would come in (both for using existing devices, and for adding new devices). I think ensuring docs are written/maintained can be enforced through pull request review. Does that make sense or have I misunderstood what you meant?

Also, the docs failed to build here. Seemed to get stuck in a loop downloading every version of PyDAQmx....that seems very, very odd!! I don't know a way to trigger the build again without pushing a new change to the PR though to find out if it was just a glitch or not.

dihm commented 3 years ago

That is a helpful clarification, but I was more thinking along the lines of practical implementation. Put more clearly than before, should I try to automate the doc building process via apidoc and custom templates or just hand code everything?

The two cases I described put the mid level docs in different files. As an example, look at the PylonCamera (the only one I have anything meaningful on so far).

Of course, looking at the build logs, it installed sphinx<2. Apidoc is not very good that far back so we should probably stick with option 1, which is fine. But that is a bit odd since the setup.cfg specifies sphinx==3.0.1 for the docs. Anyway, I'll be working on doc updates for a bit yet so there should be plenty of commits to test the doc building.

dihm commented 3 years ago

Well that is interesting. Looks like it tried to download every version of qtutils this time, ran out of versions, then promptly died trying to install labscript dependencies because of the version resolving.

Also, it somehow managed to fail to install matplotlib even though it wasn't asked for or downloaded. Did I read that right?

philipstarkey commented 3 years ago

Matplotlib is a dependency of labscript, which labscript-devices depends on. So that dependency is expected.

This time it failed due to a circular dependency that couldn't be resolved. This is probably partially due to #59, and partially due to setuptools_scm not doing what I expect with versions. I'm unclear if this is a bug in setuptools_scm, a issue with how we're using it, or an issue with my understanding (I was not heavily involved with this part of the migration), but I really feel that any commits in the master/main branch after a release branch (e.g. v3.0.x) should be detected as v3.1.0+dev... but they continue to be v3.0.0+dev...

philipstarkey commented 3 years ago

@dihm I may have worked out the versioning issue, but I'm checking with the other core devs in our chat before I make any changes (which, as a regular contributor, you're welcome to join if you are interested)

dihm commented 3 years ago

@philipstarkey I would love to join, if only to keep tabs on what is going on.

In the meantime, I'll keep chipping away at the docs here. I'll make the commits a bit larger going forward as well.

dihm commented 3 years ago

Minor question: what is the plan for devices that cannot be imported because of unlisted dependencies or dependencies on python wrappers that are proprietary/not pip installable?

The AlazarTechBoard is an offender on both counts (needs tqdm and atsapi.py), though I'm sure I'll find more as I increase coverage.

dihm commented 3 years ago

In the most recent batch, I have documented the DummyIntermediateDevice using a different organizational structure (basically everything is in the actual source file, with minimal code in the doc rst. It is an example of Case 2 I was trying to communicate earlier.

philipstarkey commented 3 years ago

@philipstarkey I would love to join, if only to keep tabs on what is going on.

In the meantime, I'll keep chipping away at the docs here. I'll make the commits a bit larger going forward as well.

Great! Could you send us an email to labscriptsuite@gmail.com with your preferred email address? I think that's all I need to send you an invite to our zulip chat.

I'll try and re-engage with the rest of this PR later today.

philipstarkey commented 3 years ago

Minor question: what is the plan for devices that cannot be imported because of unlisted dependencies or dependencies on python wrappers that are proprietary/not pip installable?

The AlazarTechBoard is an offender on both counts (needs tqdm and atsapi.py), though I'm sure I'll find more as I increase coverage.

The usual solution is to mock the proprietary APIs since they don't actually have to run. There are a few ways to do this, so we should probably google again to see what the current standard practice is. I did it once for Qt in qtutils. It might still be there (although it's technically not needed if it is since pyqt5/pyside2 can now be installed in readthedocs via pip) but it could also be out of date. Mocking entire modules automatically may require restructuring the imports as I seem to remember from blah import * being problematic. Alternatively, we could look at whether the proprietary imports should be restructured so they don't happen on device module import. For example, imports done in the BLACS worker class usually happen inside the init method, and since methods don't actually run when building docs, not having them won't be a problem if that's the only place they exist (I think).

In the most recent batch, I have documented the DummyIntermediateDevice using a different organizational structure (basically everything is in the actual source file, with minimal code in the doc rst. It is an example of Case 2 I was trying to communicate earlier.

I'm a little bit confused by the option/case numbers you are using. Do you mean this is an example of case 1, which was the second dot point in your list?

Either way, I think we probably need to have a broader chat about auto generation of API docs, what tools are out there, how good they are, and how they fit into the development workflow. We flagged this as something we needed to discuss in our dev chat when we first made the docs, so I'll wait until you've joined and we can continue there.

dihm commented 3 years ago

So looking back through the failed build logs on readthedocs, it appears the breakdown always happens during the pip install of the [docs] dependencies, right after blacs is installed. The very next package always gets stuck in a loop, no matter which one happens to be next. Not sure what that means, but it's a place to start.

dihm commented 3 years ago

So this round of docs is at a decent stopping point for now. There are a lot of details remaining, particularly in usage/installation/quirks, but I don't have the experience to fill them in for every supported device.

Since the builds continue to fail (most recent being a circular dependency problem involving BLACS thinking the current PR's labscript-devices 3.0.0rc2 as being !>=3.0.0), I'm going to move on to other module docs for the time being.

dihm commented 3 years ago

Fairly confident that merging #73 will allow these docs to finally build. Once that happens, we can hopefully get these docs merged as well.

dihm commented 3 years ago

Rebased this PR onto master to bring in the most recent changes that would hopefully fix the docs build. It did fix the circular dependency, but now there is something else. Ugh.

Edit: Looks to be an issue with how I have implemented the apidoc auto-generation of the NI-DAQmx models.

dihm commented 3 years ago

Aha!! Finally have a successful build!

Well, there is still quite a bit broken, mostly from optional modules not importing. Current list of issue modules not importing are:

Some of these are easy to fix by updating pip installable dependencies. The rest are missing proprietary code that we can't ship. Those will be harder to fix, but should be possible.

Now that there is actually something to look at, perhaps we can revisit automating docs building and style preferences.

dihm commented 3 years ago

I have fixed everything but the atsapi import. It probably isn't too hard to fix, but it would require changing how that module actually works (like putting the import inside a function that has to be called to import the library). Not exactly hard, but not sure I want to monkey around with a device that I know so little about.

I did make a change to SpinnakerCamera that move the module import into the init function, but given that is common for all the camera drivers so they can function as mock devices, I figured it was OK and assume it won't actually change anything.

dihm commented 3 years ago

I've rebased again and started the docs for the PrawnBlaster. Still need to do API docstrings.

Also, I've noticed the unordered lists don't render correctly. Apparently this is a bug in docutils==0.17 interacting with the RTD theme. This can be fixed by updating the build reqs for sphinx_rtd_theme to 0.5.2

dihm commented 3 years ago

Ready to go. Docstring coverage is 28.8%.