-
Notifications
You must be signed in to change notification settings - Fork 347
feat: Look up any instrument by name, and ensure name is unique #323
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
Changes from all commits
2afa9f2
21c9573
e1f6575
6c5c577
462cd46
290a783
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,6 +113,13 @@ def __init__(self, query_queue, response_queue, shared_kwargs): | |
| self.instruments = {} | ||
| self.next_id = 0 | ||
|
|
||
| # Ensure no references of instruments defined in the main process | ||
| # are copied to the server process. With the spawn multiprocessing | ||
| # 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 = {} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Holiday comment 🍸 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nops, hence the holiday remark in the first line.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but idk, it seems like adding one more hack makes people happy so whatevs.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also not really sure it is about inheritance, but rather namespaces. |
||
|
|
||
| self.run_event_loop() | ||
|
|
||
| def handle_new_id(self): | ||
|
|
||
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.
I am confused by the difference between
cls._all_instrumentsand
Instrument._all_instrumentson line 273, is there a difference? or should one be changed into the other?
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.
clsisInstrumenthere - even if you callfind_instrumentfrom a subclass, a@classmethodgets as an argument the class it was defined in (which I wasn't actually clear on myself, had to try it 😄 though it would still work ifclswere the subclass, it would find the object in the base class). Anyway, generally it's good practice to refer to the class by reference rather than by name, just in case the name changes later.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.
Actually, I was wrong above... made a mistake trying this out 🙈
clsis the class you called from, or the class of the instance you called from - a fact I made use of already inrecord_instance! Anyway, the conclusion stands, I will update to useclsconsistently.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.
interesting, but this sounds like sensible behavior, as long as we don't start mixing the name and
clswithout cause