lvmnps Code Review (as of commit 96efdb1)

General

Overall the code seems to be going in the correct direction but not everything works and the code is fairly difficult to understand because there are hardly any comments or docstrings.
The documentation builds and is a good starts, but it's a series of examples with little context and is not as useful as it could be. In general this is a quite simple actor and one would expect that it can be written in a way that a user that knows how to use CLU doesn't really need to read the documentation.
The tests are basically missing.
Linting is uneven, but in general the code is well formatted.
The actor commands are somehow confusing. It's not clear the difference between outlet and device. It feels as it could be organised in a much clearer way. Overall I would recommend some pen and paper writing of the command set and output datamodel.
It's not always clear when the code is using the configuration in lvmnps to get hte outlet information, and when it's reading it from the device. Only one method should be used.
If I understand the code, the idea is to run multiple instances of lvmnps, one for each type of power switch. If so, there should be a way to spin different instances of lvmnps with different names or there will be a conflict. Or will the configuration files eventually be merged into one?
I was not able to use the on/off command in the current actor running at Carnegie (see below).
The datamodel of the outputs is quite confusing and schema.json is not honoured. The datamodel requires some rethinking on paper.
Product setup and files
The product installs correctly.
There are two README files (.rst and .md). Not clear which one is the real one.
The Dockerfile looks correct.
Section 0.2.0 in the CHANGELOG.md file is misconfigured.
The .travis.yml file can be removed if Travis is not being used.
The etc/ directory at the root of the product can probably be removed. It's meant for an SDSS tool that's not likely to be used with LVM.
README.md (assumming this is the README file that's meant to be read)
The installation instructions seem more meant for a developer than for an end user. That may be ok but it should be clarified.
Not sure why sdss-clu needs to be installed. It's already a dependency of sdss-lvmnps and installed with it.
The instructions to install rabbitmq are confusing. rabbitmq is not really a dependency of lvmnps but a system-wide configuration. Again, this seems like a mix of instructions on how to setup a full system, not just lvmnps. You have the same instructions in for example lvmieb which make it look as if one needs to install rabbitmq for each piece of code.
Same with the instructions for pyenv. Pyenv itself is not required. Those instructions belong more in a development guide or development file.
The config file structure example is missing the fences. In general avoid indentation in markdown files.
In general it seems the README has become a bit the documentation. I'd recommend moving all that's not basic installation and running to the documentation itself.
pyproject.toml
Only Changgon is in the authors list. Probably Mingyeong should be added.
Remove cextern and LICENSE.md from include. They are not needed.
Update the description.
I think there are several dependencies (aiohttp, sphinx-bootstrap-theme) that are not needed. There are also depenedencies like Sphinx, black, sphinx-click, etc that are only needed for development or documentation and should not be in the dependencies section. Overall, limiting the number of dependencies as much as possible helps prevent bugs and installation issues.
The section [tool.poetry.extras] is misconfigured since sphinx-bootstrap-theme is not used and Sphinx is not an optional dependency.
.github workflows
It's unclear what the python-publish.yml file does or whether it runs. It seems to be failing now.
The release.yml file seems to do the same as python-publish.yml but it's disabled. Remove the one not used.
The docker.yml workflow seems to work fine.
The test.yml workflow fails with multiple linting issues. You can decide what level of linting and stylig you want to enforce, but should try to make sure the tests themselves always run.
lvmnps code
Overall, none of the code is commented and that makes understanding what the code is supposed to do very complicated. None of the methods or functions have docstrings indicating what the parameters are.
The PowerSwitchBase and Outlet base classes seem reasonable as far as I can understand them.
The toJson methods don't really return JSON strings, just dictionaries. That's a bit confusing.
Passing the outlet objects to update() and switch() is maybe a bit weird. I would more likely pass the names and then find them in the internal list of outlets, but since those are internal methods, I think that's fine. In general the whole thing seems a tad convoluted (for example getting the currentstatus and then using setState when one could just modify self.dli directly).
You have multiple classes all called PowerSwitch. That's likely to cause confusion down the line.
dlipower.py
Is this code used or has it been replaced with lvmpower.py? If so, probably better to remove it for clarity.
Line 124-135: not sure how those lines are used but in general it's not a good idea to put passwords in code.
lvmpower.py
I don't understand what load_configuration() does.
You are managing some exceptions with a logging of the error and in other cases by raising it. That's a bit inconsistent and may cause problems for other pieces of code that expect the object to either raise or handle the error.
I don't understand the login() method. What is it used for? You don't need to log in to the web interface to use the RESTful API. It's likely to at least duplicate the amount of time needed for each operation.
Why do you need to call add_client() on each get_url()? The client only needs to be created once.
In statuslist() I don't think you need to use BeautifulSoup. You can get all that information more reliably using the API and without adding an extra dependency. It's also a bit unclear why you need that. You're relying on your configuration file for knowing what's connected to the power switches, but here you read from the device itself. That's not a bad idea, but if that's the approach then it should all rely on the device own configuration not on lvmnps.
I don't see how you can set the value of an outlet with a GET command as seen in geturl. It may work (I haven't been able to test this because the actor running at Carnegie doesn't seem to work) but the natural way of doing that should be using a POST command.
powerswitch.py
Why do you create the DliPowerSwitch object inside isReachable()? That seems like a weird place. Also, I think if you set self.dli the first time, it will never check for reachability again, so something weird is going on there. I think you want to create the DliPowerSwitch on __init__() since you have all the information then.
I'm not sure you need to check that the device is reachable every time. Or maybe you can put that inside geturl in lvmpower.py.
exceptions.py
Maybe move this exception to get exceptions.py at the root level.
iboot/
The code here seems fine. As far as I can tell it's all asynchronous.
Actor
stop() will always fail because self._fetch_log_jobs doesn't exist.
None of the PowerSwitch classes do anything when .start() is called so not sure why you have all the timeouts and exception handling.
schema.json
You have some properties defined but you're not really using them. You don't have a global additionalProperties: false which means that anything extra that you add works and doesn't raise an error. I suggest you add the global additionalProperties: false in line 42 and you'll see tons of errors.
All keywords should be lowercase.
IndividualControl is a pretty weird name for a keyword (even if it comes from the DLI webpage). Something like "outlets" sounds much clearer.
The difference between device and switches is not clear. Also, would be better to stick to singular or plural.
You have subkeyword like info and list inside switches but those are output by themselves by the lvmnps switches command.
Commands
The difference between switches and devices is a bit confusing, especially because sometimes you call it devices and sometimes outlet.
The names of the switches (e.g., DLI-NPS-03) are a bit complicated to write.
Names of devices with spaces or complicated symbols are likely to cause issues. My advice would be to always refer to the outlets by their number and just output the name as part of the information.
Try to only use text for information to the users (as opposed to info) and error when failing a command.
device.py
You don't need to make NAME default to "". Just remove the default and the parser will take care in case somebody doesn't pass it. Same with PORTNUM, those must be mandatory commands. Also, NAME is a confusing name because it's not clear if you mean device, switch, outlet, etc.
In line 44 you are returning without failing or finishing the command.
Overall I think there are different ways in which that command can fail that are not tested.
status.py
Same commands about the defaults.
all and what (especially what) are fairly confusing command names. I would say you just need a status command with optional NAME and PORTNUM. If NAME is not passed, output everything. If NAME is passed, output all the outlets for that switch. If PORTNUM is passed, output only that specific outlet.
onoff.py

This doesn't seem to work for me at Carnegie with the develop branch running. I get, for example

lvmnps on DLI-NPS-01 7
21:37:53.535 lvmnps >
21:37:53.548 lvmnps i {
"info": "Turning on port DLI-NPS-01..."
}
21:37:53.898 lvmnps f {
"error": "Command 'on DLI-NPS-01 7' failed because of an uncaught error 'KeyError: 'DLI-NPS-01''. See traceback in the log for more information."
}

Same comment about the defaults.
Config files
Why are there copies of the config files in JSON and YAML? It's not clear which ones are used. One of them should go.
In lvmnps_dli, whic the ports have both the number as a key but also multiple num:? I'm surprised that even works.
Please, do not add the passwords to the config file. You'll need to find a way to not commit the passwords.
Whats the difference between the switch key (e.g., DLI-NPS-01) and the name, which in some cases is NULL?
Decide a place for the logs for all LVM actors. In SDSS we typically use /data/logs/<actor> but it's up to you. ~/tmp/log is not a good place.
Documentation
The documentation is the same as the README file. It has some examples but it's not really clear how things work. It needs some section explaning what outlets, devices, switches are, etc.
It's not clear if this documentation is intended for the end user/observer or for a developer. Probably the documentation needs to be organised to cover both, but clearly separated.
The API documentation is not very useful because almost no classes or methods have docstrings.
The actor command documentation builds correctly and is quite useful.
The schema documentation is not working.
The badges are broken.
Tests
There are only two tests and I cannot make any of them run.
I don't understand why you create JSONActor instances.

sdss / lvmecp

Code review v0.0.1 #18

lvmnps Code Review (as of commit 96efdb1)

General

Product setup and files

README.md (assumming this is the README file that's meant to be read)

pyproject.toml

.github workflows

lvmnps code

dlipower.py

lvmpower.py

powerswitch.py

exceptions.py

iboot/

Actor

schema.json

Commands

device.py

status.py

onoff.py

Config files

Documentation

Tests