Skip to content
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

Package organisation #1

Open
MatthieuDartiailh opened this issue Mar 2, 2015 · 39 comments
Open

Package organisation #1

MatthieuDartiailh opened this issue Mar 2, 2015 · 39 comments

Comments

@MatthieuDartiailh
Copy link
Contributor

  • Not a major question but how should we organize this package ?

    I would be in favour of organizing by vendor independently of the underlying protocol (ie visa, a dll, another python library).

  • A secondary question is what to do with hp/agilent/keysight instruments and other similar stuff ?

  • Third point as the metaclass magic can increase the lodaing time of the drivers it might be interesting to know all the existing drivers without having to import everything, and import only when the user needs a specific driver. I would love to have such a feature as when building uis I need to propose all existing drivers to my user but I like my app to start fast.

@hgrecco
Copy link

hgrecco commented Mar 2, 2015

I think by vendor is the organization that we all have reached in our packages. One package/folder per vendor is the right way to go.

They only way to address the third point is to have summary file with the most important information. This could be easily done automatically with script that the developer should run (or linked to a commit hook)

@MatthieuDartiailh
Copy link
Contributor Author

Ok so we agree about teh general organisation. I guess automating the third point is the easiest way to go, we will to thing about it.
So only remains the case of a single incrstrument existing under two vendor names (I have microwave sources which are agilent or keysight according to when they were bought). We could implement the driver in one vendor and create a dummy subclass in the other folder that is perhaps the easiest solution.

@alexforencich
Copy link
Member

Storing by vendor sounds like the consensus. Seems like some form of automation would be a good idea.

As for multiple vendor names, that is a bit more problematic. One method would be to just list everything under the current vendor name and hope it doesn't change again (sigh...HP->Agilent->Keyshite->???). Another method might be to list items under the original brand only (originally HP -> list under HP, even if there are Agilent branded units as well). Or we could do that and then create an alias of some sort under the current brand as well.

@hgrecco
Copy link

hgrecco commented Mar 3, 2015

I prefer to have it named as it is seen in the device. The rationale is that a person that is standing in front of an instrument can directly find the right driver without knowing the history of the company.

If the same device is then sold with another brand/model then we just create an new python file importing the original class with the new name (in a new packages).

@alexforencich
Copy link
Member

What about file naming? For common base classes that are used by multiple drivers, I suggest storing these in a folder called 'common'. I think the driver dev branch has 'standards' right now, but that to me indicates something along the lines of calibration standards or something along those lines and not driver components. Thoughts?

For instrument models, it is common to start with a number (e.g. 8340A), which is a no-no for a Python module name. So what is the most reasonable solution for that? I think duplicating the manufacturer name makes sense (e.g. lantz_drivers.agilent.agilent_8340A), but adding some other standard prefix may also make sense (e.g. lantz_drivers.agilent.model_8340A). Whatever we do should be a standard though, so any prefix applied would also be attached to things that don't start with a number - so you would get things like lantz_drivers.agilent.agilent_DSA91304A or lantz_drivers.agilent.model_DSA91304A. What about capitalization and delimiters? All lowercase with an underscore (agilent_dsa91304a), lowercase with uppercase model number with or without underscore (agilent_DSA91304A or agilentDSA91304A) or something else?

Also, is there a clean solution to move the actual driver object up one level in the hierarchy? As in, it would be nice to be able to import lantz_drivers.agilent.agilent_8340A and use that directly instead of having to import lantz_drivers.agilent.agilent_8340A.agilent_8340A or lantz_drivers.agilent.agilent_8340A.driver or similar. In python-ivi, this was done by importing all of the classes in the parent init.py, but this is rather tedious.

@hgrecco
Copy link

hgrecco commented Jul 24, 2015

The convention that we had until previous version of Lantz is the following:

  1. One folder for each company
  2. The folder is a (sub)package, contains and init file which imports and exposes ALL the driver classes in the package. (This can be done manually or automated)
  3. The classes are named according to the module using Python PEP8. When the model starts with a number, we prefix with the first letter of the company. I am fine to choose another (short) prefix.
  4. The number of modules in the package is really dependent on what makes sense for the company.

@alexforencich
Copy link
Member

If we can come up with a simple way to automate the import process, that would be great. However, it seems to me that the current setup is to only import things explicitly and not to import whole trees. I'm not sure how this would map on to importing all driver modules in init.py files.

I think adding prefixes in a non-consistent manner is not such a good idea. It would not be difficult to imagine a company releasing some products with numeric models, and some that happen to be prefixed with a single letter that happens to be the same as the first letter in the company name. Keysight has quite a few products with part numbers like this - Exxxx, Bxxxx, Nxxxx, Uxxxx, etc. There is no Kxxxx right now, but if they do release something like that in the future it could conflict with something that has a pure numeric model number.

@MatthieuDartiailh
Copy link
Contributor Author

I agree with @alexforencich about the prefix issue we should be consistent.

I am not a big fan of importing whole subtrees as I fear that the metaclasses magic will slow down the import. We could perhaps simply have factories in init.py with nested imports.

@alexforencich
Copy link
Member

Well, if you know how to write said factories, then that sounds like it could be the most reasonable option.

Let's figure out the prefixes so I can start writing some drivers. We're throwing a couple of undergrads at this over here at UCSD, so it would be nice if we can get these last few blocking kinks worked out soon.

@hgrecco
Copy link

hgrecco commented Jul 24, 2015

I think that we are over-engineering. Most instrumentation applications last long enough so that the import time is negligible. And we can always optimize that later. I think the important thing is to provide a convenient way, which in my opinion is:

from lantz_drivers.company import driverclass

and not:

from lantz_drivers.company.module import driverclass

This requires just importing the driver classes into the company __init__.py, which is something that you write just once and if that is very tedious you can just write a script to do it for you.

Regarding the prefixes. I am fine to use Model as in Model8340A.

@MatthieuDartiailh
Copy link
Contributor Author

My point with factory would be to have :

def GS200(*args, **kwargs):
    from . gs200 import GS200
    return GS200 (*args, **kwags)

Which we could also automatize.

But I am not sure it is true good idea.

@MatthieuDartiailh
Copy link
Contributor Author

I am fine with model as a prefix.

@hgrecco
Copy link

hgrecco commented Jul 25, 2015

My suggestion is that we can always do it later. Let's first do it in the most straightforward manner, we can always drop your solution later (which is fine for me) without any backwards compatibility problem.

Having said this, I think a better idea for the future can be something like the following in lantz_drivers/__init__.py

def get_driver_class(manufacturer, model):
     # here we look into a YAML or json  file with a tree structure, for example:
     #    agilent:
     #       8340A: lantz_drivers.agilent.somemodule.Model8340A
     #
     # This file is kept in sync with a commit hook, but you can also build upon installation

     return driver_class # which is imported according to the YAML/JSON file

I played in the past with this option and was really nice as you could easily have a single driver (without any alias classes) for whole bunch of devices that are basically the same. The import performance is really good because it only reads lantz_drivers/__init__.py and the YAML/JSON file. The drivers are really imported on demand. In any case, I think this can be done latter.

@alexforencich
Copy link
Member

I think Model is OK when an instrument starts with a number, but it seems awkward to me when it doesn't. For example, Model8340A and ModelMSO7104A vs. Agilent8340A and AgilentMSO7104A.

@alexforencich
Copy link
Member

So, if we use that get_driver_class method, can we no longer use import?

@hgrecco
Copy link

hgrecco commented Jul 25, 2015

Why not? You can always use import.

@alexforencich
Copy link
Member

So how would you use that get_driver_class method, then? import lantz_drivers.get_driver_class('Agilent', '8340A')? Or am I misunderstanding how that would work?

@MatthieuDartiailh
Copy link
Contributor Author

I really like the get_driver_class solution, as it can also allow to easily list all availables drivers. Model does look awkward to add to every name, repeating the manufacturer might be better.

@hgrecco
Copy link

hgrecco commented Jul 25, 2015

If you want the usual stuff, you do:

from lantz_drivers.company import Model38

But If you consider that the import takes too long for your script (which as I said before, is rare), you do:

from lantz_drivers import get_driver_class
C = get_driver_class('company', 'Model38')

@hgrecco
Copy link

hgrecco commented Jul 25, 2015

I really hate doing

from lant_drivers.company import Company292939

but if that is the majority decision then I am fine with it.

@MatthieuDartiailh
Copy link
Contributor Author

This won't work wih an init.py full of imports. To really gain tme you need an empty init.py.

@hgrecco
Copy link

hgrecco commented Jul 25, 2015

@MatthieuDartiailh What do you mean? lantz_drivers/__init__.py does not need any extra import

@MatthieuDartiailh
Copy link
Contributor Author

No lantz_drivers.company needs to be empty otherwise all imports are executed when importing lantz_drivers.company.model.

@hgrecco
Copy link

hgrecco commented Jul 25, 2015

You can import the module without the __init__.py as long as you do not share anything.

@hgrecco
Copy link

hgrecco commented Jul 25, 2015

But again the nice thing, in my opinion, is the aliasing and make it programatic. It is really useful for more high level appplications and when instantiating devices according to a configuration file

@alexforencich
Copy link
Member

Well, autocomplete in ipython is also nice. It's certainly something to consider, though. I can definitely see how it could be very useful.

@MatthieuDartiailh
Copy link
Contributor Author

@alexforencich you were not happy with using standards for storing the "standard" base classes. You would prefer specifications (staying close to the IVI terminology).
If we want to keep direct import possible I would prefer to have an drivers.py file per vendor in which all drivers are imported but keep the init.py file empty as otherwise importing any driver would lead to building all the classes (as the init.py found in keysight is executed when importing lantz.drivers.keysight.model_E8685A).
I will try to implement the get_driver_class function based on .yaml files, as I like aliasing the capabilities it opens and the possibility to use only the bare model name without any restrictions.

@alexforencich
Copy link
Member

Another thing that could possibly be included in .yaml files could be identifying metadata, like USB vendor and product IDs, so that it would be possible to automatically detect a device and load the correct driver. I'm not sure if this would be possible for all devices; it may not be feasible for devices that do not support the idn command.

And how about just making a folder 'base' for all of the base classes? Short, simple, to the point. Then you can have lantz.driver.base.dc_source.

@MatthieuDartiailh
Copy link
Contributor Author

I am fine with base too. As long as we do not conflict with a vendor name we are fine.
Things like usb vendor and product id can be specified in the PROTOCOLS class attributes. I should expand the docs on this. Your idea is interesting but will be indeed limited to instruments supporying *IDN.

@alexforencich
Copy link
Member

Well, the idea I had about putting them in the yaml files is that then you don't need to load the whole driver to get the value. There is probably something to be said for generating the yaml files from the python files in some way, though.

@alexforencich
Copy link
Member

Also, what about some 'standard' features that should be supported by all instruments? Things such as manufacturer, model, serial, firmware version, reset, error query, self test, etc. IVI has a whole pile of these, but I think most of them aren't really all that necessary.

@MatthieuDartiailh
Copy link
Contributor Author

I do plan to have a look at the base IVI stack but I prefer to be cautious with what we impose on instruments (as some stupid one may break). But I will definitively add some base classes to take care of that.
In dc_sources I started an scpi packages for that kind of things, I will try to expand it.

@MatthieuDartiailh
Copy link
Contributor Author

Looking at the Inherent capabilities described in IVI:

  • DriverOperation : the base_driver class cover the cache handling, everything related to interchange check we can forget (at least for the time being), we address simulation in a different fashion so I don't think we need to do anything here.
  • Identity : here we have :
    • some infos about the driver (version, supported instrument models, vendor), having a version per driver would make sense, I am more skeptical about the others.
    • some infos about the instruments (manufacturer, model, firmware) this would be nice to unify, while still getting access to the raw *IDN? value
  • Utility :
    • self_test, reset, rest_with_defaults, error_query : those three make sense but may not be available on all instruments, we could have abstract classes in base, and SCPI/IEEE488 implementations
    • disable, and lock : lock is handled as the pyvisa level and make no sense for non-visa instruments, and I have no idea what disable is meant for.

I can create abstract classes in base and some concrete in scpi (should scpi be a root level package or live in base ?). For identity, I can perhaps take advantage of stringparser to let the user simply provide the template for the IDN answer as a class attribute.

@alexforencich
Copy link
Member

That's basically the conclusion that I came to - some basic metadata - manufacturer, model, serial, firmware, possibly driver version information - and some standard actions - reset, self_test, error_query. I'm not sure if there is anything else there that makes sense.

As for SCPI, this is not a base class as it implements commands, so it should not be in the base folder. However, it could make sense to add corresponding base classes for the implemented features/actions. A top-level SCPI folder would probably make the most sense. I'm not sure about the ieee488 stuff, though, since it's common to multiple instruments but it isn't exactly SCPI. Maybe we can have a top-level folder 'common' for things like that. Or we could just throw it in the SCPI folder.

@MatthieuDartiailh
Copy link
Contributor Author

We could also have a common package and inside a ieee488 for * commands and a subpackage for scpi.

@alexforencich
Copy link
Member

That works too. Now, the next question is how best to organize the features. All top-level, or would it make sense to organize a few things into subsystems? As in, would it make sense to implement inst.model, inst.error_query, inst.reset, etc. or inst.utility.model/inst.meta.model, inst.utility.error_query, inst.utility.reset, etc.?

@MatthieuDartiailh
Copy link
Contributor Author

I think that reset, self_test, and query_error (better than error_query as this will be an action) should be at the root level, there are not that many and I don't think that list will grow. For the identity I think it would make sense to have a subsystem to avoid polluting too much the root, and I think calling it identity would be fine actually.

@MatthieuDartiailh
Copy link
Contributor Author

For the identity subsystem, I think it is fine to have more field than the instruments can potentially provide informations for (and we could simply leave them blank (returning '')), what bother me is that once again some instruments does not have reset, self_test, or error_reading capabilities (I do have such a case each command return an execution result but there is nothing such as error reading). I am in favor of keeping reset, self_test, and error_query in common but not in base. Actually I think that we won't be able to do everything with base classes but we will also need some guidelines (saying that name should be used) otherwise the number of base classes will blow up (for examples in DC sources I am currently in the nightmare of determining what should be base as basically no instrument I found (even a keysight E3631A) follow IVI recommendations).

@alexforencich
Copy link
Member

Yeah, that's one of the issues with base classes - not everything matches up exactly, so either you have to have a very large number of very small base classes so you get the required granularity, or you have to provide some way to not implement particular portions of a base class (no feature/action at all), or leave unsupported features unimplemented (feature returns none/action does nothing or raises exception, etc). None of which are ideal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants