feat: Look up any instrument by name, and ensure name is unique by alexcjohnson · Pull Request #323 · microsoft/Qcodes

alexcjohnson · 2016-09-04T13:18:06Z

Changes proposed in this pull request:

Maintain a list of all instruments by name (within one process, including RemoteInstruments). So that from the main process you'll be able to find any instrument by name, but from instrument servers you'll only be able to find those instruments on the same server. This can be done by Instrument.find_instrument(name) or, since Instrument is a parent of all other instruments, within an instrument driver you can do self.find_instrument(name).
Ensure instrument names are unique: Clearly within the lookup dictionary names must be unique, but this is fairly important also for logging. I didn't do anything to change names that already exist, I just have it throw an error. I'm a little worried about this though during experiments, as people may need to reinstantiate the same instrument for various reasons, and if we merge this people will need to explicitly close the old one before making the new one. May not be a bad requirement, just something to be aware of.

@damazter what do you think?

TODO:

Tests

damazter · 2016-09-04T14:39:09Z

Maintain a list of all instruments by name (within one process, including RemoteInstruments). So that from the main process you'll be able to find any instrument by name, but from instrument servers you'll only be able to find those instruments on the same server.

To make sure I understand this correctly:
from the main process, which instruments can I find? I would hope that only the RemoteInstruments show up (and instruments that run within the main process) is this correct?
So I would think that find_instruments should find exactly the instruments in the same process. This is regardless of wether this is the main process or an instrument server or whatever.

Secondly, this would also mean that Instrument names should only be unique per instrument server (as one instrument would usualy contain both itself, and its remote copy with the same name)

I will do a detailed reading of the changes in the near future, but I would like to understand it a bit better before I start doing that ;)

giulioungaretti · 2016-09-06T12:36:34Z

qcodes/instrument/server.py

+        # method this is not an issue, as the class is reimported in the
+        # new process, but with fork it can be a problem ironically.
+        from qcodes.instrument.base import Instrument
+        Instrument._all_instruments = {}


Holiday comment 🍸
this is a no go in my book.

It's hinting that there is a design issue somewhere. Either in the class design or in the server/proxy design.

It's a fine workaround, but shall not go into master IMHO.

Do you have an alternative to propose (other than "yes, I'm working on a new architecture")? This just exists to smooth out a difference between the inheritance models of the two multiprocessing methods, so presumably will go away entirely with the new architecture, but what do we do until then?

nops, hence the holiday remark in the first line.

but idk, it seems like adding one more hack makes people happy so whatevs.

also not really sure it is about inheritance, but rather namespaces.

alexcjohnson · 2016-09-06T13:00:33Z

@damazter

So I would think that find_instruments should find exactly the instruments in the same process. This is regardless of wether this is the main process or an instrument server or whatever.

Correct - in the main process you can find all locally-defined instruments and the RemoteInstruments pointing to all server instruments (so all instruments), and in server processes you can find all instruments that live in that process.

Secondly, this would also mean that Instrument names should only be unique per instrument server (as one instrument would usualy contain both itself, and its remote copy with the same name)

Except that the main process has references to all instruments, either as defined locally or pointing to a server (the RemoteInstrument has the same name as the instrument it proxies) so the names must be unique overall.

damazter · 2016-09-06T13:39:46Z

@alexcjohnson
Well, in my mind you could have a situation where some instruments are created on an instrument server such that the main process has no knowledge of those instruments. This might be more important conceptually than in practice though

alexcjohnson · 2016-09-06T14:25:03Z

@damazter

you could have a situation where some instruments are created on an instrument server such that the main process has no knowledge of those instruments

Yes, absolutely correct. Perhaps not to be recommended, but there's nothing stopping you.

damazter · 2016-09-09T14:53:31Z

qcodes/instrument/base.py

+        # remove from all_instruments too, but don't depend on the
+        # name to do it, in case name has changed or been deleted
+        all_ins = Instrument._all_instruments
+        for name in list(all_ins.keys()):


@alexcjohnson
Why not
for name, inst in all_ins.items():
the difference is not enormous, but this feels faster (I have no data to support this)
I think it makes the code more clear

Partially... it could be changed to

for name, inst in list(all_ins.items()): if inst is wr:

but the list() is necessary because I'm going to mutate the contents of the dict - otherwise you get RuntimeError: dictionary changed size during iteration

Ah, awesome. I learned something today

damazter · 2016-09-09T15:04:47Z

It looks good,
I will try basing my ATS driver on this PR asap, and if that works, I am happy

damazter · 2016-09-09T15:14:50Z

qcodes/instrument/base.py


+        # remove from all_instruments too, but don't depend on the
+        # name to do it, in case name has changed or been deleted
+        all_ins = Instrument._all_instruments


change it here to
ins = cls._all_instruments
then, (for consistency with line 293)?
judging by your earlier comment

good point, thanks!

Plus a couple of comments

And un-proxy find_instrument in RemoteInstrument

alexcjohnson · 2016-09-09T17:28:10Z

@damazter tested and ready when you are.

giulioungaretti · 2016-09-12T12:09:01Z

qcodes/tests/test_visa.py

        for arg in self.args3:
            self.assertIn(arg, e.exception.args)

+        mv.close()


what's this? Why added in this commit ?

Oh sorry, should have been in a previous commit. Because we can't have two instruments with the same name now, it's important to close instruments you're done with.

giulioungaretti · 2016-09-12T14:44:49Z

this should not go into v0.1, adds one more layer of things to maintain that are linked to the old architecture.

damazter · 2016-09-12T15:01:12Z

@giulioungaretti
First of all, I don't think this pr is linked to the old architecture. This PR gives instruments an interface to talk to one another, independent of architecture (the reason why this pr is better than #303)

More importantly, the alazar driver #66 is contingent on having this interface working. This PR is the last hurdle towards merging the ats driver as far as I know. I think that the alazar comes before the v0.1 milestone.

giulioungaretti · 2016-09-12T15:09:45Z

@damazter you are kind of right, but this forces the new architecture to have one more requirement that may or may not be what one wants. Maybe this is a horrible idea with the new architecture, if it's added now I will have to support it, maintain it and cry. Or maybe not. But I feel like it's too dangerous.

alexcjohnson · 2016-09-12T15:17:23Z

@giulioungaretti I can understand your reluctance... but we are going to need some way to let instruments talk to each other. Seems to me this solves a lot of issues with a clean interface and low footprint (very much unlike the clumsy shared_kwargs).

It will be important to build the new architecture in such a way that these connections can be supported, so we may as well support them from the outset, if not by this exact API then something else that we define and implement very very soon.

majacassidy · 2016-09-12T15:24:44Z

@giulioungaretti We are all waiting for the Alazar - without it there will be no v0.1 users!
Thanks!

giulioungaretti · 2016-09-12T15:26:46Z

@majacassidy @alexcjohnson . Then go for it, I will just suffer if it was not the right choice.
Thanks!

damazter · 2016-09-15T11:25:35Z

@alexcjohnson
I tested this with the Alazar driver. All seems to be working except that there is a new sort of error which might occour:
If I have instrument 'a' running on server 'A' and i want instrument 'b' to have a reference to 'a' but I started b with server_name=None then it will find instrument 'a' by name as a remote instrument (which is not what I wanted because I wanted a reference to instrument 'a', not the remote copy)

This is stupid behaviour of the user in this case, but the crash it causes is nasty and obscure.
I can think of two solutions. either we rename all remote instruments (e.g. ats_remote)
Or I would build in a check when looking up an instrument such to make sure that I was not given a remote instrument (or e.g. a keithley for that matter)

what do you think?

alexcjohnson · 2016-09-15T15:18:40Z

@damazter good point. Two thoughts:

we could log a warning if the returned object is a RemoteInstrument. This would only happen in the main process, unless the user does something really unusual, so the warning should be clearly visible.
we could accept the desired instrument class as an optional argument, and raise a (clear) error if it's the wrong class. This would silence the RemoteInstrument warning, if you explicitly ask for a RemoteInstrument

damazter · 2016-09-16T07:58:29Z

@alexcjohnson
Thinking about this for a bit I think I like the second option a lot more. I don't know yet why a user might want a reference to a remote instrument but it seems to ad hoc to me to display a warning when a user does want this.

What I like about the second option is that it also prevents the user from finding a weird instrument because of some naming issue. I think it would be a good idea to add this to this pr, such that developers are alerted to this potential mistake.

alexcjohnson · 2016-09-16T14:46:49Z

@damazter great, #2 it is: no selectivity or warnings if you omit the second arg, raises an error if the second arg doesn't match the found instrument. Ready to 💃 ?

giulioungaretti · 2016-09-16T15:56:17Z

let's hope this won't create subtle problems with the future 🕶 (more state to take care of, but Alazar comes first! )

damazter · 2016-09-19T17:41:45Z

qcodes/instrument/remote.py

        """
        return self._instrument_class.instances()

+    def find_instrument(self, name):


What is the reason for overwriting the baseclass method?

Oh smart, nevermind. Never thought about remote instrument not being an instrument :d

damazter · 2016-09-19T17:55:58Z

@alexcjohnson
Instrument Alazar1 is <class 'qcodes.instrument.remote.RemoteInstrument'> but <class 'qcodes.instrument_drivers.AlazarTech.ATS.AlazarTech_ATS'> was requested
So that is wonderful (and my driver works if I put them on the same server). So I am 100% happy.
💃 if you ask me

Merge: 0989a14 290a783 Author: alexcjohnson <johnson.alex.c@gmail.com> Merge pull request #323 from qdev-dk/feat/find-inst

feat: Look up any instrument by name, and ensure name is unique

2afa9f2

giulioungaretti reviewed Sep 6, 2016
View reviewed changes

damazter reviewed Sep 9, 2016
View reviewed changes

alexcjohnson added 2 commits September 9, 2016 12:12

style: consistent use of cls instead of explicit class reference

21c9573

Plus a couple of comments

test: For find_instrument and friends

e1f6575

And un-proxy find_instrument in RemoteInstrument

giulioungaretti reviewed Sep 12, 2016
View reviewed changes

giulioungaretti added this to the v0.2 essential extracts milestone Sep 12, 2016

alexcjohnson added 3 commits September 16, 2016 10:32

feat: class testing in find_instrument / find_component

6c5c577

docs: docstring for find_component + instrument_class

462cd46

fix: my TypeError was throwing a TypeError!

290a783

giulioungaretti mentioned this pull request Sep 16, 2016

qcodes instruments are not thread safe #327

Open

damazter reviewed Sep 19, 2016

View reviewed changes

alexcjohnson merged commit 332688a into master Sep 19, 2016

alexcjohnson deleted the feat/find-inst branch September 19, 2016 19:04

damazter pushed a commit that referenced this pull request Sep 20, 2016

fix: adapt to new find_inst structure of pr #323

2ef46df

giulioungaretti pushed a commit that referenced this pull request Sep 20, 2016

Generated gh-pages for commit 332688a

2e82b0c

Merge: 0989a14 290a783 Author: alexcjohnson <johnson.alex.c@gmail.com> Merge pull request #323 from qdev-dk/feat/find-inst

damazter mentioned this pull request Sep 21, 2016

feat: add kwarg to instrument constructor on server #303

Closed

giulioungaretti mentioned this pull request Sep 30, 2016

Failing instrument creation adds instrument to list of instruments #348

Closed

giulioungaretti mentioned this pull request Dec 12, 2016

refer to instruments from another instrument #185

Closed

Conversation

alexcjohnson commented Sep 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damazter commented Sep 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson commented Sep 6, 2016

Uh oh!

damazter commented Sep 6, 2016

Uh oh!

alexcjohnson commented Sep 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damazter commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giulioungaretti commented Sep 12, 2016

Uh oh!

damazter commented Sep 12, 2016

Uh oh!

giulioungaretti commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcjohnson commented Sep 12, 2016

Uh oh!

majacassidy commented Sep 12, 2016

Uh oh!

giulioungaretti commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damazter commented Sep 15, 2016

Uh oh!

alexcjohnson commented Sep 15, 2016

Uh oh!

damazter commented Sep 16, 2016

Uh oh!

alexcjohnson commented Sep 16, 2016

Uh oh!

giulioungaretti commented Sep 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damazter commented Sep 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexcjohnson commented Sep 4, 2016 •

edited

Loading

giulioungaretti commented Sep 12, 2016 •

edited

Loading

giulioungaretti commented Sep 12, 2016 •

edited

Loading